[DomCrawler] add a value() method, normalize whitespaces #24412

xabbuh · 2017-10-03T16:40:55Z

Q	A
Branch?	4.1
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21302, #23626
License	MIT
Doc PR	TODO

Tobion · 2017-10-03T17:42:10Z

src/Symfony/Component/DomCrawler/Tests/CrawlerTest.php

+    {
+        $this->assertEquals("one   two\nthree   ", $this->createTestCrawler()->filterXPath('//div[@id="text-whitespaces"]')->value(), '->value() returns the node value of the first element of the node list');
+
+        $this->createTestCrawler()->filterXPath('//ol')->value();


this should be a separate test like testValueThrowsExceptionWhenNodeListEmpty. if the above call aleady fails with InvalidArgumentException, the test would still pass.

src/Symfony/Component/DomCrawler/Crawler.php

vlakoff · 2017-10-03T17:59:26Z

So, on 3.4 people will have to update their codebase to text(true), then in 4.0 they will have to update it again... We're not very kind to them.

Couldn't we drop the 3.4 step and just make the 4.0 change? I don't really see the benefits of the 3.4 step, apart from adding work.

xabbuh · 2017-10-03T18:05:57Z

There is no need to update the code in 4.0 again. You could omit the argument then, but it's not necessary.

ro0NL · 2017-10-03T18:41:23Z

UPGRADE-3.4.md

+----------
+
+ * added an argument to the `text()` method to opt-in normalizing whitespaces, not passing `true` is deprecated, for the
+   previous behavior of the `text()` method, use the new `value()` method


* Added an argument to the `text()` method to opt-in normalizing whitespaces, not passing `true` is deprecated. For the previous behavior of the `text()` method, use the new `value()` method.

? (see dots)

ro0NL · 2017-10-03T18:41:51Z

UPGRADE-4.0.md

+DomCrawler
+----------
+
+ * the `text()` method now normalizes whitespaces of the node text value


same. Compared to above/below blocks this should be sentences no?

src/Symfony/Component/DomCrawler/Crawler.php

nicolas-grekas · 2017-10-04T06:54:48Z

I'm sorry I don't get the migration path at all.
I understand we're fighting adding a flag... By adding a flag...
As far as the migration path is concerned, this will require ppl to change their code twice: once when moving to 4.0, by adding a flag (failed goal already)
And twice in 4.1 to remove the deprecation...

If we must add a flag because there is no other migration path, let's settle on having a flag and keep it. This would mean zero deprecation (but for ppl extending the class, but that's edgy.)

nicolas-grekas · 2017-10-04T06:57:04Z

Oh, and we could then, in 4.1, deprecate the flag and replace it by this value() method.
That'd mean one deprecation, thus one migration.
Better, isn't it?

nicolas-grekas · 2017-10-04T06:59:16Z

src/Symfony/Component/DomCrawler/Crawler.php

+    public function text($normalizeWhitespaces = false)
+    {
+        if (!$normalizeWhitespaces) {
+            @trigger_error(sprintf('Not normalizing whitespace characters when using the %s() method is deprecated since version 3.4. The %s() method will return the first node\'s value with whitespaces being normalized in Symfony 4.0. Use the value() method instead if you do not want to have whitespaces normalized.', __METHOD__, __METHOD__), E_USER_DEPRECATED);


This deprecation is not required and should be removed.
Also, the added arg is a bc break right now, should be done using func_get_arg()

xabbuh · 2017-10-04T09:41:04Z

Fair enough, I reverted the deprecation.

Status: Needs Review

nicolas-grekas

That's still strange, sorry :)
What about deprecating test(), and add a $normalizeWhitespace = true flag to value()
The flag is a requirement to me, I can't find any better path that would be migration-path-friendly.
But by defaulting to "true", value could just be good by default, WDYT?

xabbuh · 2017-10-06T09:22:38Z

I understand what you mean, but if we did that, value() would have to get passed false to behave as I would have expected. I mean the name indicates returning the raw value, doesn't it? So if we want to go the way with the flag, I suggest to just add it to text() and then decide somewhere along the 4.x if we deprecate the flag and introduce something else or just live with the flag.

stof · 2017-10-06T09:35:04Z

I the context of an HTML crawler, I think that value() would be confusing it. If I have a crawler holding an input element or a select option, I would understand ->value() as returning the value of the input (which is in an attribute), not the inner text (which is what ->nodeValue does).
FYI, the JS property corresponding to this property is textContent.

The normalization you are doing here for text corresponds to part of the behavior non-standard innerText property of JS DOM APIs (we cannot reproduce the full behavior, as this involves removing text from non-rendered children and we are not doing rendering, and reproducing other parts cannot be done with a simple regex: https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute)

xabbuh · 2017-10-06T11:05:07Z

I suggest to postpone this PR for Symfony 4.1 to not rush anything into core with which we may not be completely satisfied.

fabpot · 2018-10-10T12:14:21Z

@xabbuh Time to resurrect this PR?

fabpot · 2019-02-21T10:23:11Z

@xabbuh Still interested in finishing this PR?

vlakoff · 2019-02-22T13:45:57Z

To avoid all that upgrade path mess with the method parameters, I'd suggest just adding a new method as I proposed initially.

Simperfit · 2019-07-03T06:38:56Z

@xabbuh do you want me to finish this one ?

xabbuh · 2019-07-06T09:35:50Z

@Simperfit If you have some time, please feel free to take over. 👍

fabpot · 2019-07-08T10:06:58Z

@Simperfit Is this PR still on your radar?

Simperfit · 2019-07-08T10:08:07Z

@fabpot doing it today Le lun. 8 juil. 2019 à 12:07, Fabien Potencier <notifications@github.com> a écrit :

…

@Simperfit <https://github.com/Simperfit> Is this PR still on your radar? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24412?email_source=notifications&email_token=AA2KV4QR2NX6V3BY3S3QHXLP6MGVPA5CNFSM4D5RWEZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZMTYZA#issuecomment-509164644>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2KV4QZWOSVZ5VKUC5ALUTP6MGVPANCNFSM4D5RWEZA> .

xabbuh · 2019-07-08T17:01:23Z

closing in favour of #32440

xabbuh · 2019-07-08T17:01:51Z

Thank you for taking over @Simperfit!

@xabbuh

…t() method (Simperfit) This PR was merged into the 4.4 branch. Discussion ---------- [DomCrawler] add a normalizeWhitespace argument to text() method | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #21302, #23626  | License | MIT | Doc PR | todo   I've taken #24412, rebased with 4.4, fixed the comments in the PR. cc @xabbuh @stof @nicolas-grekas Commits ------- 1746718 [DomCrawler] add a value() method, normalize whitespaces

xabbuh added this to the 3.4 milestone Oct 3, 2017

carsonbot added Status: Needs Review DomCrawler Feature Deprecation labels Oct 3, 2017

xabbuh mentioned this pull request Oct 3, 2017

[DomCrawler] Add method flatText() #23626

Closed

xabbuh force-pushed the issue-21302 branch from 26860d6 to a3ce9dc Compare October 3, 2017 16:46

Tobion reviewed Oct 3, 2017

View reviewed changes

vlakoff reviewed Oct 3, 2017

View reviewed changes

src/Symfony/Component/DomCrawler/Crawler.php Show resolved Hide resolved

ro0NL reviewed Oct 3, 2017

View reviewed changes

xabbuh force-pushed the issue-21302 branch from a3ce9dc to 1228845 Compare October 3, 2017 19:30

carsonbot removed the Status: Needs Review label Oct 4, 2017

ro0NL approved these changes Oct 4, 2017

View reviewed changes

carsonbot added the Status: Reviewed label Oct 4, 2017

javiereguiluz approved these changes Oct 4, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 4, 2017

nicolas-grekas requested changes Oct 4, 2017

View reviewed changes

xabbuh force-pushed the issue-21302 branch from 1228845 to 0a12b4c Compare October 4, 2017 09:37

xabbuh removed the Deprecation label Oct 4, 2017

xabbuh force-pushed the issue-21302 branch from 0a12b4c to aae8dca Compare October 4, 2017 09:40

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 4, 2017

add a value() method, normalize whitespaces

cdea8b2

xabbuh force-pushed the issue-21302 branch from aae8dca to cdea8b2 Compare October 4, 2017 09:57

nicolas-grekas reviewed Oct 4, 2017

View reviewed changes

xabbuh modified the milestones: 3.4, 4.1 Oct 6, 2017

xabbuh changed the base branch from 3.4 to master November 22, 2017 10:10

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

Simperfit mentioned this pull request Jul 8, 2019

[DomCrawler] add a normalizeWhitespace argument to text() method #32440

Merged

xabbuh closed this Jul 8, 2019

xabbuh deleted the issue-21302 branch July 8, 2019 17:02

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DomCrawler] add a value() method, normalize whitespaces #24412

[DomCrawler] add a value() method, normalize whitespaces #24412

xabbuh commented Oct 3, 2017 •

edited by javiereguiluz

Loading

Tobion Oct 3, 2017

vlakoff commented Oct 3, 2017

xabbuh commented Oct 3, 2017

ro0NL Oct 3, 2017

ro0NL Oct 3, 2017

nicolas-grekas commented Oct 4, 2017 •

edited

Loading

nicolas-grekas commented Oct 4, 2017

nicolas-grekas Oct 4, 2017

xabbuh commented Oct 4, 2017

nicolas-grekas left a comment

xabbuh commented Oct 6, 2017

stof commented Oct 6, 2017

xabbuh commented Oct 6, 2017

fabpot commented Oct 10, 2018

fabpot commented Feb 21, 2019

vlakoff commented Feb 22, 2019

Simperfit commented Jul 3, 2019

xabbuh commented Jul 6, 2019

fabpot commented Jul 8, 2019

Simperfit commented Jul 8, 2019 via email

xabbuh commented Jul 8, 2019

xabbuh commented Jul 8, 2019

[DomCrawler] add a value() method, normalize whitespaces #24412

[DomCrawler] add a value() method, normalize whitespaces #24412

Conversation

xabbuh commented Oct 3, 2017 • edited by javiereguiluz Loading

Tobion Oct 3, 2017

Choose a reason for hiding this comment

vlakoff commented Oct 3, 2017

xabbuh commented Oct 3, 2017

ro0NL Oct 3, 2017

Choose a reason for hiding this comment

ro0NL Oct 3, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Oct 4, 2017 • edited Loading

nicolas-grekas commented Oct 4, 2017

nicolas-grekas Oct 4, 2017

Choose a reason for hiding this comment

xabbuh commented Oct 4, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

xabbuh commented Oct 6, 2017

stof commented Oct 6, 2017

xabbuh commented Oct 6, 2017

fabpot commented Oct 10, 2018

fabpot commented Feb 21, 2019

vlakoff commented Feb 22, 2019

Simperfit commented Jul 3, 2019

xabbuh commented Jul 6, 2019

fabpot commented Jul 8, 2019

Simperfit commented Jul 8, 2019 via email

xabbuh commented Jul 8, 2019

xabbuh commented Jul 8, 2019

xabbuh commented Oct 3, 2017 •

edited by javiereguiluz

Loading

nicolas-grekas commented Oct 4, 2017 •

edited

Loading