Add a non-static API for the CssSelector component #15934

stof · 2015-09-26T22:55:05Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#15850, #8404
License	MIT
Doc PR	todo

This implements a non-static API for the CssSelector component.

I decided to keep the static API too, as it is convenient when you just need a one-shot conversion (if you need lots of conversions, keeping a reference to the Converter and all its internal object graph may be faster than releasing it all the time and rebuilding it).
I deprecated the global state to choose between HTML and XML conversion. The static API would always enable the HTML extension in 3.0. Dealing with XML would be done by using the Converter class.

A second commit also tags all internal classes of the component as @internal, as there is really no reason for a user to deal with them (btw, we already considered them fully internal in the past, as we broke BC on them in a patch release to fix memory performance of the component in the past).

TODOs:

Validate whether we keep the static facade to the component
send a PR on the documentation to document this new API.
handle usage of the deprecated API in the DomCrawler testsuite

The DomCrawler component does not use the new API yet. I will do it in a separate PR, as distinguishing between HTML and XML modes for a crawler will be easier once I deprecate the possibility to load multiple documents (which I will do tomorrow).

stof · 2015-09-28T10:33:19Z

@symfony/deciders please give your mind on this

jakzal · 2015-09-28T10:50:59Z

Looks good to me 👍

I would get rid of the static API as I prefer to have one way of doing the same thing. CssSelector::toXpath('h2'); vs (new Converter())->toXpath('h2'); is not much of a difference.

xabbuh · 2015-09-28T10:53:47Z

src/Symfony/Component/CssSelector/ConverterInterface.php

+     *
+     * @return string
+     *
+     * @api


Why not tagging the whole interface instead?

should be removed. we're not tagging from the get-go

@Tobion IMO, it makes sense to tag it now, as this is the API we expect them to use (or we should drop this tagging altogether).

the api tag just means it has stricter BC rules. it doesn't make sense to enforce that on a new class without usage experience. otherwise we could add it everywhere.

Well, we haven't tagged anything as @api since 2.3 AFAICT. And we still work hard on BC for all these classes.

Btw, the api tag BC rules are not stricter. they are the real rules. the special rules are for non-api classes, where we allow BC breaks, but only some of them. And as we became better at writing BC layers and worse at tagging things as @api, this distinction does not really make sense IMO. We already apply strict BC almost everywhere

I agree with what you say. Maybe it's time to remove the api tags then.

xabbuh · 2015-09-28T10:57:42Z

Looks good to me and I agree with @jakzal that we can get rid of the static API in 3.0.

fabpot · 2015-09-28T11:06:06Z

👍 for removing the static API as well in 3.0.

fabpot · 2015-09-28T11:06:36Z

src/Symfony/Component/CssSelector/Converter.php

+    {
+        return $this->translator->cssToXPath($cssExpr, $prefix);
+    }
+


blank line should be removed

Tobion · 2015-09-28T13:47:35Z

👍 for deprecating the static method

stof · 2015-09-28T13:53:33Z

The PR is updated.

As the static API is now deprecated, I switched the DomCrawler component to use the non-static API already. Currently, it switched to XML mode as soon as you add XML content (and is HTML by default), which closes #8404
As soon as #15849 is done (this evening), we will not have the possibility to load both XML and HTML content in the same crawler, making this unambiguous.

I extracted the creation of sub crawlers (created by filtering an existing one) to a dedicated private method to avoid duplication of the code copying the state in multiple places in Symfony. this is something I would have done anyway as I will have more state to copy when doing the DomCrawler changes. This is cleaner than adding new constructor arguments used only internally (it would clutter the public API). Btw, this allowed me to spot a place where the baseHref property was not passed to the new crawler properly.

Tobion · 2015-09-28T14:02:11Z

src/Symfony/Component/CssSelector/CssSelector.php

    }

    /**
     * Enables the HTML extension.
+     *
+     * @deprecated as of 2.8, will be removed in 3.0. Use the \Symfony\Component\CssSelector\Converter class instead.


the class is already deprecated

All these classes are meant to be considered as an implementation detail. A normal usage of the component does not require to deal with them at all.

xabbuh · 2015-09-28T20:22:19Z

src/Symfony/Component/CssSelector/ConverterInterface.php

+ *
+ * @author Christophe Coevoet <stof@notk.org>
+ *
+ * @api


should be removed now

same with the tag on the method of course

fabpot · 2015-09-28T20:26:45Z

Thank you @stof.

This PR was merged into the 2.8 branch. Discussion ---------- Add a non-static API for the CssSelector component | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #15850, #8404 | License | MIT | Doc PR | todo This implements a non-static API for the CssSelector component. I decided to keep the static API too, as it is convenient when you just need a one-shot conversion (if you need lots of conversions, keeping a reference to the Converter and all its internal object graph may be faster than releasing it all the time and rebuilding it). I deprecated the global state to choose between HTML and XML conversion. The static API would always enable the HTML extension in 3.0. Dealing with XML would be done by using the Converter class. A second commit also tags all internal classes of the component as ``@internal``, as there is really no reason for a user to deal with them (btw, we already considered them fully internal in the past, as we broke BC on them in a patch release to fix memory performance of the component in the past). TODOs: - [x] Validate whether we keep the static facade to the component - [ ] send a PR on the documentation to document this new API. - [x] handle usage of the deprecated API in the DomCrawler testsuite The DomCrawler component does not use the new API yet. I will do it in a separate PR, as distinguishing between HTML and XML modes for a crawler will be easier once I deprecate the possibility to load multiple documents (which I will do tomorrow). Commits ------- 9e51279 [CssSelector] Tag all internal classes as internal ones f4563c3 Add a non-static API for the CssSelector component

xabbuh · 2015-09-28T20:27:52Z

src/Symfony/Component/DomCrawler/composer.json

@@ -20,7 +20,7 @@
    },
    "require-dev": {
        "symfony/phpunit-bridge": "~2.7|~3.0.0",
-        "symfony/css-selector": "~2.3|~3.0.0"
+        "symfony/css-selector": "~2.8|~3.0.0"
    },
    "suggest": {
        "symfony/css-selector": ""


Should we expand this to explain that we need the CssSelector component in version 2.8 or higher?

xabbuh · 2015-09-28T20:30:15Z

I extracted the creation of sub crawlers (created by filtering an existing one) to a dedicated private method to avoid duplication of the code copying the state in multiple places in Symfony. this is something I would have done anyway as I will have more state to copy when doing the DomCrawler changes. This is cleaner than adding new constructor arguments used only internally (it would clutter the public API). Btw, this allowed me to spot a place where the baseHref property was not passed to the new crawler properly.

What about backporting this change to Symfony 2.3? At least, we should fix the slice() method there.

Tobion · 2015-09-28T20:37:17Z

By comments have not been taken into account:

what do we need to Interface for? Will there be different implementations?

Imo the class name is too generic. How about CssToXPathConverter?

fabpot · 2015-09-28T20:41:02Z

I did not read your comment about the interface; indeed, I'm in favor of removing this interface as I don't see how we could have another implementation.

fabpot · 2015-09-29T06:19:13Z

@Tobion see #15988 for the interface removal.

This PR was merged into the 2.8 branch. Discussion ---------- [CssSelector] remove ConverterInterface | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15934 | License | MIT | Doc PR | n/a Commits ------- 8b8b634 [CssSelector] updated README fd3fefb [CssSelector] remove ConverterInterface

@stof

Make sure that all relevant information is passed to created crawlers. To avoid future regressions, this commit backports the approach taken by @stof in symfony#15934 to have a single place in the class that is responsible to create subcrawler instances.

@stof

Make sure that all relevant information is passed to created crawlers. To avoid future regressions, this commit backports the approach taken by @stof in symfony#15934 to have a single place in the class that is responsible to create subcrawler instances.

@stof

This PR was merged into the 2.7 branch. Discussion ---------- [DomCrawler] always pass base href to subcrawlers | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Make sure that all relevant information is passed to created crawlers. To avoid future regressions, this commit backports the approach taken by @stof in #15934 to have a single place in the class that is responsible to create subcrawler instances. Commits ------- 3d9a748 [DomCrawler] always pass base href to subcrawlers

carsonbot added the Status: Needs Review label Sep 26, 2015

stof added the CssSelector label Sep 26, 2015

stof force-pushed the css_selector_non_static branch 2 times, most recently from b132a47 to 2bcfc42 Compare September 27, 2015 13:37

xabbuh reviewed Sep 28, 2015
View reviewed changes

fabpot reviewed Sep 28, 2015
View reviewed changes

stof force-pushed the css_selector_non_static branch from 2bcfc42 to 28eb5b3 Compare September 28, 2015 13:47

stof mentioned this pull request Sep 28, 2015

CssSelector should be configured properly when used for HTML or XML #8404

Closed

Tobion reviewed Sep 28, 2015
View reviewed changes

stof added 2 commits September 28, 2015 16:03

Add a non-static API for the CssSelector component

f4563c3

[CssSelector] Tag all internal classes as internal ones

9e51279

All these classes are meant to be considered as an implementation detail. A normal usage of the component does not require to deal with them at all.

stof force-pushed the css_selector_non_static branch from 28eb5b3 to 9e51279 Compare September 28, 2015 14:03

xabbuh reviewed Sep 28, 2015
View reviewed changes

fabpot merged commit 9e51279 into symfony:2.8 Sep 28, 2015

xabbuh reviewed Sep 28, 2015
View reviewed changes

fabpot mentioned this pull request Sep 29, 2015

[CssSelector] remove ConverterInterface #15988

Merged

stof deleted the css_selector_non_static branch September 29, 2015 07:36

xabbuh mentioned this pull request Sep 30, 2015

[DomCrawler] always pass base href to subcrawlers #16028

Merged

xabbuh mentioned this pull request Oct 12, 2015

Documenting the new CSS selector API symfony/symfony-docs#5782

Closed

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a non-static API for the CssSelector component #15934

Add a non-static API for the CssSelector component #15934

stof commented Sep 26, 2015

stof commented Sep 28, 2015

jakzal commented Sep 28, 2015

xabbuh Sep 28, 2015

Tobion Sep 28, 2015

stof Sep 28, 2015

Tobion Sep 28, 2015

stof Sep 28, 2015

Tobion Sep 28, 2015

stof Sep 28, 2015

xabbuh commented Sep 28, 2015

fabpot commented Sep 28, 2015

fabpot Sep 28, 2015

Tobion commented Sep 28, 2015

stof commented Sep 28, 2015

Tobion Sep 28, 2015

stof Sep 28, 2015

xabbuh Sep 28, 2015

xabbuh Sep 28, 2015

fabpot commented Sep 28, 2015

xabbuh Sep 28, 2015

xabbuh commented Sep 28, 2015

Tobion commented Sep 28, 2015

fabpot commented Sep 28, 2015

fabpot commented Sep 29, 2015

Add a non-static API for the CssSelector component #15934

Add a non-static API for the CssSelector component #15934

Conversation

stof commented Sep 26, 2015

stof commented Sep 28, 2015

jakzal commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Sep 28, 2015

fabpot commented Sep 28, 2015

Choose a reason for hiding this comment

Tobion commented Sep 28, 2015

stof commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 28, 2015

Choose a reason for hiding this comment

xabbuh commented Sep 28, 2015

Tobion commented Sep 28, 2015

fabpot commented Sep 28, 2015

fabpot commented Sep 29, 2015