Deprecate Zend\Dom\Query in favor of more logical OO approach

[tca3]

Hello everyone,

I am quite a heavy user of DOMDocument XPath queries and I have always been bothered by the Dom implementation in ZF2. The Zend\Dom\Query class seems very illogical as an object and contains quite a lot of inefficiencies and lacks access to the used DOMDocument generated from a string.

I created Zend\Dom\Document which makes more sense, because we are querying this object and not the other way around (creating a Query object that generates a DOMDocument which we then query and that we can't even access).

Zend\Dom\Query class:

$query = new Query($html);
$query->execute('a'); // Generation of a DOMDocument
$query->execute('span'); // Generation of another DOMDocument from the same input string
$query->getDOMDocument(): // Impossible

Zend\Dom\Document class:

$document = new Document($html);
$document->query('a', Document::QUERY_CSS);
$document->query('//span', Document::QUERY_XPATH);
$dom      = $document->getDomDocument();
$dom->createTextNode('<node></node>');

Problems solved:

• Addition of a method to get the created DOMDocument instance
• Caching of generated DOMDocument on each query
• More logical object (Document VS Query)
• Only one method to query a document

The methods more or less stay the same, I reused the tests already available for Query and added some for the new behavior. I deprecated Zend\Dom\Query in favor of this new class.

I'm in favor of adding this to a ZF2.* future release because I consider this more like an a new feature (new logic & different object) than a component refactor. ZF3 is very far away and Zend\Dom is currently lacking a proper DOMDocument wrapper.

[blanchonvincent]

docblock missing

[blanchonvincent]

👍 nice refactoring. Maybe we have to keep this work for ZF3 ? cc @Maks3w

[tca3]

@blanchonvincent It does qualify for a refactoring, I agree, but if we want to fully do this, we would need to change NodeList as well because I can't for the life of me figure out why it needs a cssQuery and an xPathQuery in the constructor. I would need to adress that as well as the CSS2Xpath class that should be a simple static method...

But I'd really like to see this in the next ZF2 release though :(

[blanchonvincent]

@ThomasCantonnet I think it's too important to have this in the next release. And, yes, I have the same opinion, you need to change NodeList (you can have cssQuery && xpathQuery when you use the queryCss, because the cssQuery is transformed to a xpathQuery :) ).

[samsonasik]

@return self

[tca3]

yup ok

[blanchonvincent]

@ThomasCantonnet Maybe you can have a Zend\Dom\Document (with encoding, errors, document transformation to domdocument) and a Zend\Dom\Query (cssQuery & xpathQuery) which will use a Zend\Dom\Document.

Your code will be more decoupled and easier to test & maintain.

[tca3]

@blanchonvincent Having an object just to store 2 properties and a NodeList seems like a lot of unncessary overhead to me, don't you think ? In this case all we need to do is rename NodeList to Query

[blanchonvincent]

@ThomasCantonnet no because the "setStringDocument" and the "getDomDocument" logic will be moved into a same class, and the query logic will be on an other class.

[blanchonvincent]

@ThomasCantonnet SRP = Single Responsibility Principle :)

[tca3]

Can you give me more details on how you view this new architecture? I'm not sure I'm following.

[sasezaki]

When getDomDoument() called twice, getStringDocument() called twice ?

can be ?

if (null === $this->domDocument) {
    if (null === ($stringDocument = $this->getStringDocument())) {

[tca3]

getStringDocument() is a simple pointer to the property $stringDocument.

[weierophinney]

A few things to note:

• If you have completely new classes, this is completely Backwards Compatible (BC), meaning we can push it into 2.3.0.
• I agree with @blanchonvincent -- have one class for Document, another for Query (I'd call the new one Zend\Dom\Document\Query to keep BC). This keeps the responsibilities clear, and makes testing easier, as Blanchon noted.
• I also agree with Blanchon that a public setter for the document string/object doesn't make sense. This should be passed to the constructor, and remain immutable. If you want to query a different document, you should create a new instance.
• Finally, once the design is solid, let's also update Zend\Test to use the new functionality. :)

[tca3]

@weierophinney Thanks for your input, I'll look into it during the week if I have time.

On Dom\Query though, should I change all the functions while keeping BC or create a different class ? I'm not sure I can keep BC with the state of Dom\Query.

[tca3]

@weierophinney @blanchonvincent Alright guys, deprecated a lot of classes but it looks much cleaner now :)

I added a Query object and refactored the NodeList object, what do you guys think about it now?

$document = new Document($html, Document::DOC_XHTML, 'UTF-8');
$query    = new Document\Query('.foo', Document\Query::TYPE_CSS);
$result   = $document->execute($query);
echo $result->count();

[weierophinney]

@ThomasCantonnet I think the class NodeList must be only a container, NodeList must be have only the node list, not the factory method, no logic business. Put the logic business in the execute method in Query class.

After reviewing, I agree with this. You can remove the factory method from NodeList, and then have the execute method of Query prepare the data to inject into the NodeList.

[weierophinney]

@ThomasCantonnet the parameter of the method execute is a document, strange ...

I disagree, @blanchonvincent. Look at how PHP's DOM library works, particularly DOMXPath::query -- that accepts the expression and the DOM node (or document).

With that said, @ThomasCantonnet - it may make sense to make that mirroring even more clear, and maybe do the following:

• Remove the constructor to the Query class
• Make the execute method signature execute($expression, Document $document, $type = static::TYPE_XPATH). Then parse the $expression if we have a CSS expression, before handing it off to DOMXPath.

Usage would then become:

$document = new Document($contents);
$nodelist    = Query($someExpression, $document, Query::TYPE_CSS);
foreach ($nodelist as $node) {
    // ...
}

Or, more succinctly, for those who like Perl golf:

foreach (Query($someExpression, new Document($contents), Query::TYPE_CSS) as $node) {
    // ...
}

Both of which look quite clean to me.

[tca3]

OK guys, I removed the NodeList factory method and changed Query::execute to be a static method with Matthew's proposition for a signature.

Do we agree now ? :)

[weierophinney]

Merged to develop for release with 2.3.0. @ThomasCantonnet -- can you do a new PR that will update Zend\Test\PHPUnit to use the new API, please?

[tca3]

Sure, I'll see what I can do!

Le 1 nov. 2013 à 22:33, weierophinney notifications@github.com a écrit :

Merged to develop for release with 2.3.0. @ThomasCantonnet -- can you do a new PR that will update Zend\Test\PHPUnit to use the new API, please?

—
Reply to this email directly or view it on GitHub.

[Maks3w]

@ThomasCantonnet @weierophinney I already made the PR for fix Zend\Test. See #5356

[Maks3w]

Well in true I fixed the deprecated call which made ZendTest\Test tests fail

[jmleroux]

The documentation needs an update.