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

[DomCrawler] fixed bug #12143 #12283

Closed
wants to merge 1 commit into from
Closed

[DomCrawler] fixed bug #12143 #12283

wants to merge 1 commit into from

Conversation

dkop
Copy link

@dkop dkop commented Oct 22, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12143
License MIT
Doc PR -

@@ -176,13 +182,13 @@ public function addHtmlContent($content, $charset = 'UTF-8')

$baseHref = current($base);
if (count($base) && !empty($baseHref)) {
if ($this->uri) {
if ($this->baseHref) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever be false given that it was not empty?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 184 $baseHref, on line 185 $this->baseHref

@wouterj
Copy link
Member

wouterj commented Nov 5, 2014

This replaces #12144, could you please add the tests from that PR here?

Btw, many thanks for working on this one! I really hope this is merged soon, as it's one of the blockers for my company to start using Behat...

@fabpot
Copy link
Member

fabpot commented Nov 17, 2014

Looks good to me but tests from #12144 do not pass after applying these changes.

@dkop Can you integrate the tests from #12144?

@dkop
Copy link
Author

dkop commented Nov 17, 2014

@fabpot Ok, i added tests from #12144.
Last data line in dataProvider getBaseTagData was incorrect in these tests.
@wouterj Sorry for delay, please confirm mistake in your test.

@wouterj
Copy link
Member

wouterj commented Nov 24, 2014

@dkop it is wrong for <a>, but correct for <form>: http://jsbin.com/laxugelire/2/quiet

This is actually what the DomCrawler is doing wrong, resulting in failed tests for me.

@fabpot
Copy link
Member

fabpot commented Dec 12, 2014

@dkop Will you have time to finish this one? It looks like that we are almost there.

@dkop
Copy link
Author

dkop commented Dec 12, 2014

@fabpot as i can understand it is already done.

array('http://base.com' /* base */, '' /* href */, 'http://base.com' /* expectedUri */, 'http://domain.com/path/link', '<base> tag does work with empty links'),

This line of tests is correct right now according to the test in browsers: http://jsbin.com/laxugelire/2/quiet
This line https://github.com/symfony/symfony/pull/12144/files#diff-116347d1689bd54d19e1f9a6901ef8a7R843 is incorrect

@wouterj
Copy link
Member

wouterj commented Dec 12, 2014

@dkop it is incorrect for links, but correct for form. Atm, the form inherits all url logic from link. You should change Form class to use the special form logic.

@fabpot
Copy link
Member

fabpot commented Dec 29, 2014

closing in favor of #13145

@fabpot fabpot closed this Dec 29, 2014
fabpot added a commit that referenced this pull request Dec 29, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

[DomCrawler] Fix behaviour with <base> tag

Finishes #12283

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12283, #12143, #12144
| License       | MIT
| Doc PR        | -

Commits
-------

91447e8 Make fabbot happy
1d35e48 Clean up testing
61f22d7 [DomCrawler] fixed bug #12143
fabpot added a commit to symfony/dom-crawler that referenced this pull request Dec 29, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

[DomCrawler] Fix behaviour with <base> tag

Finishes symfony/symfony#12283

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12283, #12143, #12144
| License       | MIT
| Doc PR        | -

Commits
-------

91447e8 Make fabbot happy
1d35e48 Clean up testing
61f22d7 [DomCrawler] fixed bug #12143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants