Skip to content

Fix dom class can't be inherited by the internal class #2263

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

Closed
wants to merge 1 commit into from
Closed

Fix dom class can't be inherited by the internal class #2263

wants to merge 1 commit into from

Conversation

dreamsxin
Copy link
Contributor

No description provided.

@nikic
Copy link
Member

nikic commented Dec 30, 2016

What's the background for this change?

@dreamsxin
Copy link
Contributor Author

dreamsxin commented Dec 31, 2016

@nikic When create new extension define class MyDOMElement extends DOMElement:

zend_register_internal_class_ex(&my_ce, dom_element_class_entry);

Happen error PHP Warning: Couldn't fetch MyDOMElement. Node no longer exists in test.php on line 9 When:

<?php
$html = <<<HTML
<a href="a">a</a>
HTML;
$document = new \DOMDocument();
$document->registerNodeClass("\\DOMElement", "\\MyDOMElement");
$document->loadHTML(html);
$node = $document->childNodes->item(1);
var_dump($node, $node->nodeName);

@krakjoe
Copy link
Member

krakjoe commented Dec 31, 2016

Comparing class entries directly looks and feels wrong ...

@nikic
Copy link
Member

nikic commented Dec 31, 2016

Maybe check against the module entry of the class instead?

@dreamsxin
Copy link
Contributor Author

dreamsxin commented Dec 31, 2016

If type equal ZEND_INTERNAL_CLASS, The intern->prop_handler = zend_hash_find_ptr(&classes, base_class->name); will equal NULL.

@nikic
Copy link
Member

nikic commented Jan 1, 2017

To clarify, what I meant is checking for something like base_class->type == ZEND_INTERNAL_CLASS && base_class->info.internal.module == &dom_module_entry.

@dreamsxin
Copy link
Contributor Author

@nikic Thank you, I changed.

php-pulls pushed a commit that referenced this pull request Jan 1, 2017
php-pulls pushed a commit that referenced this pull request Jan 1, 2017
php-pulls pushed a commit that referenced this pull request Jan 1, 2017
@krakjoe
Copy link
Member

krakjoe commented Jan 1, 2017

Please check tests ...

php-pulls pushed a commit that referenced this pull request Jan 1, 2017
@dreamsxin
Copy link
Contributor Author

@krakjoe Sorry, Now it's OK.

@smalyshev smalyshev added the Bug label Jan 1, 2017
@nikic
Copy link
Member

nikic commented Jan 1, 2017

Re-merged via 3c97761, thanks!

@nikic nikic closed this Jan 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants