Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PHP warnings on Deploy Package #625

Merged
merged 3 commits into from
Jan 31, 2025
Merged

Conversation

eduardomozart
Copy link
Contributor

When opening a package to edit and into another tab switch the profile to Self-Service (or any other profile which doesn't have access to Deploy Package page) it throws a PHP Warning on page if display errors is enabled on PHP settings.

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes # (issue number, if applicable)
  • Here is a brief description of what this PR does

Screenshots (if appropriate):

image

When opening a package to edit and into another tab switch the profile to Self-Service (or any other profile which doesn't have access to Deploy Package page) it throws a PHP Warning on page if display errors is enabled on PHP settings.
@trasher
Copy link
Collaborator

trasher commented Jan 31, 2025

Displaying error from PHP config is strongly discouraged.

If user do not have access to page, I guess trying to display it is incorrect.

@stonebuzz
Copy link
Collaborator

do you use nginx?
If yes

Check nginx configuration, especially

fastcgi_param SERVER_NAME $host;

From /etc/nginx/fastcgi_params

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Jan 31, 2025

Displaying error from PHP config is strongly discouraged.

If user do not have access to page, I guess trying to display it is incorrect.

Sorry, I think I express myself badly (my native language isn't English). I have access to the page. What happened is the following:

  1. Access the Deploy Package page as Super-Admin (with proper rights).
  2. Into another browser tab, change the profile to Self-Server (no proper rights anymore).
  3. Change to Super-Admin again into that another browser tab.
  4. Go back/reload the Deploy Package browser tab (with proper rights).

As it expects a referrer, if it isn't set, the PHP function strstr() throws an error as the HTTP_REFERRER cannot be null (the first parameter of strstr cannot be null actually) to determine which Massive Actions to shown on Actions dropdown.

This PR is innofensive as such check was already being made before (it only checks if HTTP_REFERRER is set before calling strstr()) and keep the current behavior of the code, just a PHP 8 fix.

@trasher
Copy link
Collaborator

trasher commented Jan 31, 2025

[...]
4. Go back/reload the Deploy Package page (with proper rights).

OK, sounds more legit, thanks for details.

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Jan 31, 2025

Hello @trasher, thank you for your suggestions, but I believe your changes won't be compatible with PHP 7.4 since strstr() accepts an empty string only from PHP 8.0 and onwards.

@trasher
Copy link
Collaborator

trasher commented Jan 31, 2025

As far

Hello @trasher, thank you for your suggestions, but your changes won't be compatible with PHP 7.4 since strstr() accepts an empty string only from PHP 8.0 and onwards.

No, that will works. In PHP >=8, this function only accepts a string, and not the null value. In PHP 7.4, it accepts both.

 % php74 -r '$array=[]; var_dump(strstr($array["value"] ?? "", "test"));'
bool(false)
 % php82 -r '$array=[]; var_dump(strstr($array["value"] ?? "", "test"));'
bool(false)

eduardomozart and others added 2 commits January 31, 2025 10:16
Co-authored-by: Johan Cwiklinski <trasher@x-tnd.be>
Co-authored-by: Johan Cwiklinski <trasher@x-tnd.be>
@eduardomozart
Copy link
Contributor Author

I see. I commited your changes. Thank you!

@trasher trasher merged commit c17dbab into glpi-project:main Jan 31, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants