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] Cloned Crawler objects not usable because of overwritten getHash method #16421

Closed
wasinger opened this issue Nov 2, 2015 · 5 comments

Comments

@wasinger
Copy link

wasinger commented Nov 2, 2015

Since 2.8, DomCrawler overrides the SplStorage method "getHash($object)" for the purpose of displaying a deprecation message.

When a Crawler object is cloned this triggers PHP Bug #67582 Cloned SplObjectStorage with overwritten getHash fails offsetExists() and makes the cloned Crawler instance not working correctly (e.g. it's not possible to remove elements from them because methods detach() and removeAll() don't work).

Removing elements from cloned Crawler instances was not a problem up to Symfony 2.7. It's a regression that breaks some of my projects.

The problem was introduced in commit 997c650

@jakzal
Copy link
Contributor

jakzal commented Jan 5, 2016

@wasinger can you give me an example how to reproduce this behaviour?

@wasinger
Copy link
Author

Here is how to reproduce it:

use Symfony\Component\DomCrawler\Crawler;

$c = new Crawler('<html><head></head><body>Some HTML code</body></html>');
echo "Crawler has " . count($c) . " Elements\n"; // 1
$c1 = clone $c;
echo "Cloned Crawler has " . count($c1) . " Elements\n"; // 1

// Remove first node from Crawler
$c->detach($c->getNode(0));
echo "Crawler has " . count($c) . " Elements\n"; // 0

// Remove first node from cloned Crawler
$c1->detach($c1->getNode(0));
echo "Cloned Crawler has " . count($c1) . " Elements\n"; // Should be: 0, sf 2.7: 0, sf 2.8: 1

The last line prints "1" in Symfony 2.8 while it should be 0.

@jakzal
Copy link
Contributor

jakzal commented Feb 10, 2016

Options I see here:

  • Acknowledge this is a PHP bug and do nothing.
  • Remove the overridden getHash() method. It means we wouldn't trigger a deprecation if someone called this method directly. To be honest I think it's quite unlikely that anyone would rely on getHash() without relying on other methods from the \SplObjectStorage.
  • Don't call parent methods of \SplObjectStorage's, but replicate their behaviour with an array of nodes. This might get messy and introduce new bugs.

@stof
Copy link
Member

stof commented Feb 10, 2016

I suggest going for the second option

@jakzal
Copy link
Contributor

jakzal commented Feb 10, 2016

Ok. I'll send a PR soon.

jakzal added a commit that referenced this issue Feb 10, 2016
…vent problems when cloning the crawler (jakzal)

This PR was merged into the 2.8 branch.

Discussion
----------

[DomCrawler] Remove the overridden getHash() method to prevent problems when cloning the crawler

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

Overriding the `SplObjectStorage::getHash()` is affected by a [PHP bug](https://bugs.php.net/bug.php?id=67582), which makes the Crawler unusable in Symfony 2.8 for anyone who relied on `SplObjectStorage` methods.

Removing the `getHash()` method means we will no longer trigger the deprecation error. Given this method is unlikely to be used directly and other `SplObjectStorage` methods will trigger the error, it is the simplest thing we can do to maintain BC.

Commits
-------

3d7f6c6 [DomCrawler] Remove the overridden getHash() method to prevent problems when cloning the crawler
@jakzal jakzal closed this as completed Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants