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

Updates setVersionHeader() to check specific meta #9

Merged
merged 1 commit into from
Feb 18, 2016
Merged

Updates setVersionHeader() to check specific meta #9

merged 1 commit into from
Feb 18, 2016

Conversation

legshooter
Copy link
Contributor

Right now, the meta tag that's being filtered from the request by setVersionHeader() is actually all http-equiv meta tags.

As I had <meta http-equiv="X-UA-Compatible" content="IE=edge"> in my project's <head>, it kept capturing IE=edge.

So with this change instead, it will now look for a specific <meta http-equiv="X-PJAX-Version" content="foo"> tag, to enable the correct version to be passed on to the response.

If this is accepted, I'll PR the Readme with a cool tip about using Laravel's elixir, to handle force reload automatically.

@freekmurze
Copy link
Member

Hi,

As you can see from the test results this is a breaking change. You can make this a non-breaking change by first checking on the specific meta tag.

$node = $crawler->filter('head > meta[http-equiv="X-PJAX-Version"]');

If no node is found the code should try to get the node with:

$node = $crawler->filter('head > meta[http-equiv]');

Could you make this change and add a test for it to prove that it works?

Feel free to improve the readme with your tip in a separate PR.

Thanks!

@legshooter
Copy link
Contributor Author

Hi Freek,

I see it now. Don't mean to be a party pooper, but I don't know how to handle testing. I frankly can't even say I understand the reasoning, since getting some other random http-equiv meta tag doesn't sound like a fitting fallback. But that could be due to my lack of experience with OS, I am guessing this refers to the BC part.

freekmurze added a commit that referenced this pull request Feb 18, 2016
Updates setVersionHeader() to check specific meta
@freekmurze freekmurze merged commit 5f151d1 into spatie:master Feb 18, 2016
@freekmurze
Copy link
Member

After looking into the issue a bit deeper, I've decided to treat this as a bugfix release. I'll adjust the tests and put a note in the change log.

@legshooter
Copy link
Contributor Author

Yay, my first OS code contribution <3

@freekmurze
Copy link
Member

Congrats! 🎉Please keep on contributing to our repo's and all others 👍

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.

2 participants