Skip to content

Crawler default namespace fix #9771

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

Closed
wants to merge 3 commits into from

Conversation

crudecki
Copy link

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #9732, #6650
License MIT
Doc PR symfony/symfony-docs/pull/2979

Fix backwards compatibility of xml namespaces for having only one default namespace.

@ghost
Copy link

ghost commented Dec 15, 2013

ubot detected some small issues in this pull request.

You can make the modifications by hand or run the following command from the repository root directory:

curl http://ubot.io/patch/symfony/symfony/9771.diff | patch -p0
diff -ru src/Symfony/Component/DomCrawler/Crawler.php src/Symfony/Component/DomCrawler/Crawler.php
--- src/Symfony/Component/DomCrawler/Crawler.php    2013-12-15 09:28:56.199122873 +0000
+++ src/Symfony/Component/DomCrawler/Crawler.php    2013-12-15 09:28:56.535122873 +0000
@@ -201,7 +201,7 @@
     public function addXmlContent($content, $charset = 'UTF-8')
     {
         // remove the default namespace if it's the only namespace to make XPath expressions simpler
-        if(!preg_match('/xmlns:/', $content)) {
+        if (!preg_match('/xmlns:/', $content)) {
             $content = str_replace('xmlns', 'ns', $content);
         }

@fabpot
Copy link
Member

fabpot commented Dec 15, 2013

ping @jakzal

@jakzal
Copy link
Contributor

jakzal commented Dec 16, 2013

@fabpot I've helped with this PR during the hack day so I'm 👍

This seems to be the only way to revert the BC break partially. This PR brings back the hack which we used to have, but only for the scenario when there's a single namespace and it is a default namespace. So it simplifies filtering for single namespace documents and keeps it flexible for multiple namespace ones.

It might be worth documenting. I'll send a PR if we all agree it's good to merge.

@fabpot
Copy link
Member

fabpot commented Dec 16, 2013

@jakzal You're the boss here, so let's document this properly.
@crudecki Can you rebase your PR to remove the merge commit and avoid any non-related commits?

@jakzal
Copy link
Contributor

jakzal commented Dec 16, 2013

@crudecki also, ubot suggest a small CS fix ;)

@jakzal
Copy link
Contributor

jakzal commented Dec 16, 2013

I think we should also update the CHANGELOG.md:

`Crawler::addXmlContent()` removes the default document namespace again if it's an only namespace.

@crudecki
Copy link
Author

I've removed the unnecessary merge

@fabpot
Copy link
Member

fabpot commented Dec 17, 2013

@crudecki Great, can you add the suggested change in the CHANGELOG?

@crudecki
Copy link
Author

Done it through Github interface, hope this is fine.

@jakzal
Copy link
Contributor

jakzal commented Dec 17, 2013

Doing so through github interface results in a new PR: #9800

@stloyd
Copy link
Contributor

stloyd commented Dec 17, 2013

@crudecki Go at this page: https://github.com/crudecki/symfony/blob/crawlerDefaultNamespaceFix/src/Symfony/Component/DomCrawler/CHANGELOG.md and edit file there, after that it will be in this PR so you can close #9800.

Added changelog message for the Crawler::addXmlContent() BC break fix with one default namespace.
@crudecki
Copy link
Author

Done : ) help in getting started with contributing much appreciated : ) thanks

@jakzal
Copy link
Contributor

jakzal commented Dec 17, 2013

🍺

fabpot added a commit that referenced this pull request Dec 17, 2013
This PR was squashed before being merged into the 2.4 branch (closes #9771).

Discussion
----------

Crawler default namespace fix

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9732, #6650
| License       | MIT
| Doc PR        | symfony/symfony-docs/pull/2979

Fix backwards compatibility of xml namespaces for having only one default namespace.

Commits
-------

cfff054 Crawler default namespace fix
@fabpot fabpot closed this Dec 17, 2013
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.

4 participants