[DomCrawler] Add method flatText() #23626

vlakoff · 2017-07-22T14:12:31Z

Q	A
Branch?	3.4 (to be rebased from master)
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21302
License	MIT
Doc PR	symfony/symfony-docs#...

Similar to text(), with some normalization applied to the value: surrounding spaces/newlines are removed, and consecutive spaces/newlines are reduced to one. Helpful if you don't want your code to choke because of the HTML source formatting.

This is a follow-up to #11470 and #21302. ping @javiereguiluz, @jpetitcolas.

Quoting #21302 (comment):

Personally I'm extending the Crawler class, I couldn't anymore clutter my code all over with this:

$ref = preg_replace('/\s+/', ' ', trim($node
    ->filter('div.numero_ref')
    ->text()));

Now I just have to do:

$ref = $node
    ->filter('div.numero_ref')
    ->cleanText();

vlakoff · 2017-07-22T14:26:24Z

What version number should be used for adding an entry to the changelog?

nicolas-grekas · 2017-07-25T12:49:50Z

3.4 (and the PR be rebased + retargetted to branch 3.4 if you can.)

vlakoff · 2017-07-25T13:56:38Z

Added changelog entry, and rebased to 3.4 (I had followed the instructions, they tell to submit to master…)

stof · 2017-07-25T15:57:06Z

@vlakoff these instructions are true for 18 months when we don't develop x.4 and x+1.0 in parallel. during the 6 months where we prepare the next minor and the next major, we have 2 active development branch (and anything which is backward compatible goes into the next minor)

vlakoff · 2017-07-25T23:20:13Z

Another thing, should there be a PR for adding this method to the documentation here, or is better to not add clutter and have it as a "hidden feature"?

javiereguiluz · 2017-07-26T06:37:22Z

@vlakoff there should be a PR for docs ... but only if this proposal is finally approved. So let's wait a bit.

javiereguiluz · 2017-07-26T06:38:27Z

src/Symfony/Component/DomCrawler/Crawler.php

+    public function flatText()
+    {
+        if (!$this->nodes) {
+            throw new \InvalidArgumentException('The current node list is empty.');


I wonder if it's OK to throw an exception in this case. Would it be better to simply return an empty string?

I'd prefer the exception =/ same as text() does.

Note this method can be a simple one-liner return trim(preg_replace('/\s+/', ' ', $this->text()));

Throwing an exception is consistent with the rest of this class, which indeed I think is quite strict in practice (in my code I have to add a lot of tests for possibly absent nodes, instead of just getting null).

Couldn't it be an argument to the existing text() method instead?

javiereguiluz · 2017-07-26T06:50:22Z

src/Symfony/Component/DomCrawler/Crawler.php

+     *
+     * @throws \InvalidArgumentException When current node is empty
+     */
+    public function flatText()


I don't like the proposed function name (flatText()) because this is usually a verb ("flatten") and here we're proposing to use an adjective ("flat"). I've been looking for in the standard libraries of some programming languages (Go, JavaScript, Java, C#, ...) but nobody defines a function like that (they only implement something like our trim(), ltrim(), rtrim(), etc.) Even Laravel doesn't define anything like this in Illuminate\Str.

Maybe trimAll() would be a better name? Or flatten()? Or trimText(), flattenText()? I'm not sure.

It's more a getter than an action, so I think a noun rather than a verb is fine (and actually better).

By the way, I had thought about flattenedText, but it's too long.

You're flattening text().. it's an action :) i'd also favor flattenText() here.

ro0NL · 2017-07-26T08:57:57Z

Note JS does this by default (element.innerText || element.textContent) :) maybe we can patch text() instead, and optionally add rawText() as well.

stof · 2017-07-26T09:13:39Z

We cannot change the behavior of text(). This would be a BC break.

ro0NL · 2017-07-26T09:20:40Z

True. Just saying it might be better to update it's behavior instead, if this is about DX.

We could bridge the BC with a temp. arg text($flatten = false) i guess,, not sure if worth it ;)

vlakoff · 2017-08-02T06:05:44Z

Thoughts about new method vs. adding a parameter ? (notice the latter isn't strictly BC, if user were extending the method)

nicolas-grekas · 2017-09-26T15:02:25Z

Turning my -1 into a -0, ping @symfony/deciders it's up to us now.

javiereguiluz · 2017-09-26T15:16:19Z

I'm 👍 about the feature (truly useful and needed very often) but I'm still 👎 about the method name (flatText()). It doesn't feel OK to me.

nicolas-grekas · 2017-09-28T09:02:52Z

@javiereguiluz any suggestion to help move forward?

javiereguiluz · 2017-09-28T09:41:19Z

I've asked in Symfony Slack, and the community has suggested these alternative names:

stripSpace()
trimSpaces()
strip()
spaceless()

javiereguiluz · 2017-09-28T09:50:32Z

Maybe we can follow @ro0NL proposal and wait until 4.0 to make a BC break? We could have:

text() would work like this proposal and return you just the text contents
rawText() (originalText() ?) which is like the current text() and returns things unchanged

xabbuh · 2017-09-28T10:25:11Z

@javiereguiluz But even with a BC break in 4.0 we would need an upgrade path. We could do so with a new $stripSpaces argument defaulting to false that would keep the current behaviour while passing true would strip spaces. We could then change the default value to true in 4.0 and entirely deprecate the argument in 4.1. The current behaviour of text() could then be mimicked by a new method like rawText() or so.

vlakoff · 2017-09-28T14:21:40Z

It seems like things are getting complicated only because we can't figure out a proper name for the function...

I'm unhappy with some of the proposals above, because the function does two things, it doesn't only trim. About reversing the function behaviors, this still can be done later.

I just thought of a new one, how about tidyText()?

Pierstoval · 2017-09-28T17:58:24Z

Maybe clean(ed?)Text() ?

jakzal · 2017-10-03T12:39:26Z

We could do so with a new $stripSpaces argument defaulting to false that would keep the current behaviour while passing true would strip spaces. We could then change the default value to true in 4.0 and entirely deprecate the argument in 4.1. The current behaviour of text() could then be mimicked by a new method like rawText() or so.

As much as I'm usually against such flags, in this case I agree it's our best choice.

fabpot · 2017-10-03T13:36:06Z

The flag sounds like the best idea, indeed.

vlakoff · 2017-10-03T14:46:29Z

Added a commit to change to the "flag" way.

Comments welcome about any rewording, nitpicking, etc.
Then I could squash it, and rebase it if needed.

Tobion · 2017-10-03T15:13:14Z

Don't use a flag for this unless for deprecation.

text($stripSpaces = false) with deprecating $stripSpaces = false
value() for old behavior because it's also nodeValue

vlakoff · 2017-10-03T15:46:54Z

I understood your point about using the flag only for deprecation.

But TBH, handling all this transitional code is getting incredibly boring to me. So I'm closing this, if someone wants to take it over, he would be very welcome.

xabbuh · 2017-10-03T16:41:08Z

see #24412 as a replacement

@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

carsonbot added Status: Needs Review DomCrawler Feature labels Jul 22, 2017

vlakoff force-pushed the flatText branch 2 times, most recently from 10857ea to 92ee667 Compare July 22, 2017 14:21

vlakoff force-pushed the flatText branch from 92ee667 to 47fa769 Compare July 22, 2017 22:03

nicolas-grekas added this to the 3.4 milestone Jul 25, 2017

vlakoff force-pushed the flatText branch from 47fa769 to 1745549 Compare July 25, 2017 13:53

vlakoff changed the base branch from master to 3.4 July 25, 2017 13:54

javiereguiluz reviewed Jul 26, 2017

View reviewed changes

vlakoff added 2 commits October 3, 2017 16:48

[DomCrawler] Add method flatText()

0793daf

Add a flag to text() instead of adding a method

71e8b51

vlakoff force-pushed the flatText branch from 8626be0 to 71e8b51 Compare October 3, 2017 14:49

vlakoff closed this Oct 3, 2017

xabbuh mentioned this pull request Oct 3, 2017

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

Closed

Simperfit mentioned this pull request Jul 8, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DomCrawler] Add method flatText() #23626

[DomCrawler] Add method flatText() #23626

vlakoff commented Jul 22, 2017 •

edited by nicolas-grekas

Loading

vlakoff commented Jul 22, 2017

nicolas-grekas commented Jul 25, 2017

vlakoff commented Jul 25, 2017

stof commented Jul 25, 2017

vlakoff commented Jul 25, 2017

javiereguiluz commented Jul 26, 2017

javiereguiluz Jul 26, 2017

ro0NL Jul 26, 2017 •

edited

Loading

vlakoff Jul 26, 2017 •

edited

Loading

fabpot Aug 1, 2017

javiereguiluz Jul 26, 2017

vlakoff Jul 26, 2017 •

edited

Loading

ro0NL Jul 26, 2017

ro0NL commented Jul 26, 2017

stof commented Jul 26, 2017

ro0NL commented Jul 26, 2017

vlakoff commented Aug 2, 2017

nicolas-grekas commented Sep 26, 2017

javiereguiluz commented Sep 26, 2017

nicolas-grekas commented Sep 28, 2017

javiereguiluz commented Sep 28, 2017

javiereguiluz commented Sep 28, 2017

xabbuh commented Sep 28, 2017

vlakoff commented Sep 28, 2017

Pierstoval commented Sep 28, 2017

jakzal commented Oct 3, 2017

fabpot commented Oct 3, 2017

vlakoff commented Oct 3, 2017

Tobion commented Oct 3, 2017

vlakoff commented Oct 3, 2017

xabbuh commented Oct 3, 2017

[DomCrawler] Add method flatText() #23626

[DomCrawler] Add method flatText() #23626

Conversation

vlakoff commented Jul 22, 2017 • edited by nicolas-grekas Loading

vlakoff commented Jul 22, 2017

nicolas-grekas commented Jul 25, 2017

vlakoff commented Jul 25, 2017

stof commented Jul 25, 2017

vlakoff commented Jul 25, 2017

javiereguiluz commented Jul 26, 2017

javiereguiluz Jul 26, 2017

Choose a reason for hiding this comment

ro0NL Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

vlakoff Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot Aug 1, 2017

Choose a reason for hiding this comment

javiereguiluz Jul 26, 2017

Choose a reason for hiding this comment

vlakoff Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

ro0NL Jul 26, 2017

Choose a reason for hiding this comment

ro0NL commented Jul 26, 2017

stof commented Jul 26, 2017

ro0NL commented Jul 26, 2017

vlakoff commented Aug 2, 2017

nicolas-grekas commented Sep 26, 2017

javiereguiluz commented Sep 26, 2017

nicolas-grekas commented Sep 28, 2017

javiereguiluz commented Sep 28, 2017

javiereguiluz commented Sep 28, 2017

xabbuh commented Sep 28, 2017

vlakoff commented Sep 28, 2017

Pierstoval commented Sep 28, 2017

jakzal commented Oct 3, 2017

fabpot commented Oct 3, 2017

vlakoff commented Oct 3, 2017

Tobion commented Oct 3, 2017

vlakoff commented Oct 3, 2017

xabbuh commented Oct 3, 2017

vlakoff commented Jul 22, 2017 •

edited by nicolas-grekas

Loading

ro0NL Jul 26, 2017 •

edited

Loading

vlakoff Jul 26, 2017 •

edited

Loading

vlakoff Jul 26, 2017 •

edited

Loading