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 EZP-22982: Treemenu throws PHP notices with nginx #1111

Merged
merged 1 commit into from
Nov 13, 2014

Conversation

yannickroger
Copy link
Contributor

Link: https://jira.ez.no/browse/EZP-22982

Description

First of all, this patch is more of a workaround than a fix. It avoids the notice to happen, but doesn't optimize the behavior as it is done using apache.

$_SERVER['PHP_SELF'] is not processed the same way in apache (with mod_rewrite) and nginx. Apache applies the rewrite rules on the returned value while nginx doesn't apply them returning a result matching what is specified in the doc http://php.net/manual/en/reserved.variables.server.php

For example requesting http://ez5.local/ezdemo_site_admin the value of $_SERVER['PHP_SELF'] would be:

  • apache: /ezdemo_site_admin/
  • nginx: index.php/ezdemo_site_admin

Ways to fix it:

  • in ezpublish legacy code, replace the usage of $_SERVER['PHP_SELF'] by $_SERVER['REQUEST_URI']: That could lead to a lot of BC breaks
  • in nginx config, tweak the setting to make it behave the way apache does. This is not very clean and might lead to other problems using new and cleaner code.

That's why I decided in this PR to go with the safest approach (a workaround) since we are dealing with legacy code.

Test

Manual test (on front and back) and ran the install wizard.

@crevillo
Copy link
Contributor

I was talking about this with @bdunogier at the same moment. what a coincidence :). Are we sure treemenu is the cause? Can't assure but if i'm not wrong the error is happening also in the login page, where theorically treemenu isn't used...

@yannickroger
Copy link
Contributor Author

it happens in a bunch of places :) But it has been reported using the treemenu.

@crevillo
Copy link
Contributor

then ok :).

@andrerom
Copy link
Contributor

+1 (5.3, 5.4 and master) if it works :)

@yannickroger
Copy link
Contributor Author

"it works on my computer"©
:) but QA will make sure it works everywhere.

@crevillo
Copy link
Contributor

works on mine too. ubuntu.

@bdunogier
Copy link
Member

+1 ! I'm bored of having to add '@' on line 1238 of ezsys.php every now and then ;-)

yannickroger added a commit that referenced this pull request Nov 13, 2014
Fix EZP-22982: Treemenu throws PHP notices with nginx
@yannickroger yannickroger merged commit b461ae3 into master Nov 13, 2014
@yannickroger yannickroger deleted the ezp-22982_nginx_php_notices branch November 13, 2014 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants