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

Swap apache_request_headers() for $_SERVER #811

Merged
merged 1 commit into from Nov 2, 2016
Merged

Swap apache_request_headers() for $_SERVER #811

merged 1 commit into from Nov 2, 2016

Conversation

da2x
Copy link
Contributor

@da2x da2x commented Oct 23, 2016

Apache isn’t the only server in town anymore, and there is no reason to use this method rather than the superglobal variable.

@philwareham
Copy link
Member

@Bloke @bloatware @vanmelick is this OK to merge? Not my area.

@da2x
Copy link
Contributor Author

da2x commented Nov 1, 2016

Reviving this should only take five seconds, if anyone is up for it. The patch simply replaces three uses of the apache_request_headers() function with the server-neutral $_SERVER superglobal.

@Bloke
Copy link
Member

Bloke commented Nov 2, 2016

Honestly I don' t know what A_IM is or does, nor can I find any reference to it on the Internet. But if Aeyoun says it's available in $_SERVER, and we need to still use it, then who am I to argue? The superglobal approach is fine by me, since it doesn't appear we're using apache_request_headers() for any other special values.

But, as I say, I have zero knowledge in what this bit of code does for Txp.

@da2x
Copy link
Contributor Author

da2x commented Nov 2, 2016

Every HTTP request header is in $_SERVER servers. The difference from apache_request_headers() is that the superglobal prefixes headers with HTTP_ and that it is available in every web server environment and not just those claiming to be Apache. This patch just makes sure everyone is using the same variables from the same place regardless of server environment. Should reduce the number environment-unique bugs.

Now that I’m looking more closely into the feed delta implementation (A-IM) in Textpattern; I see that it’s actually entirely broken. That is a separate issue, however. I can fix that in a few days time, it shouldn’t be much work at all.

@Bloke
Copy link
Member

Bloke commented Nov 2, 2016

Thanks for the clarification. And if you can fix the feed implementation as part of this request then that'd be brilliant. It's the dark arts to me.

Would you prefer we merge this one and then you can make a separate PR for the next fix or do you want to add more commits to this one? I'm happy either way.

@da2x
Copy link
Contributor Author

da2x commented Nov 2, 2016

Just merge this, and I’ll get around to the other thing later this week. 👍
Incremental improvements and all that.

@makss
Copy link
Contributor

makss commented Nov 2, 2016

It is advisable to rewrite the code in atom.php and rss.php, very much there is out of date and no longer makes sense. Can be deleted:

  • completely block apache_request_headers() or $_SERVER["HTTP_A_IM"]
  • completely block $cutarticles and related headers. (Internet is not running on the modems and such complexities in order to save traffic, I do not see the point.)
  • Remove function safe_hed
  • Remove function fixup_for_feed

Simplified calculation Etag
It seems that there still can simplify sql queries.

@Bloke Bloke merged commit 92d0752 into textpattern:dev Nov 2, 2016
@da2x
Copy link
Contributor Author

da2x commented Nov 3, 2016

Pull request #831 takes care of the remainder, as promised.

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.

None yet

4 participants