Skip to content

Promote warnings to exceptions in ext/simplexml - #6011

Closed
kocsismate wants to merge 5 commits into
php:masterfrom
kocsismate:simplexml-warning
Closed

Promote warnings to exceptions in ext/simplexml#6011
kocsismate wants to merge 5 commits into
php:masterfrom
kocsismate:simplexml-warning

Conversation

@kocsismate

@kocsismatekocsismate commented Aug 18, 2020

Copy link
Copy Markdown
Member

No description provided.

@GirgiasGirgias mentioned this pull request Aug 18, 2020
Closed
71 tasks
nikic
nikic reviewed Aug 24, 2020
View reviewed changes
Comment threadext/simplexml/simplexml.c Outdated

@nikicnikicAug 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error condition seems to be untested.

@kocsismatekocsismateAug 24, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, I couldn't trigger this error case in the newly added test :/

Comment threadext/simplexml/simplexml.c Outdated
Comment threadext/simplexml/simplexml.c Outdated
Comment threadext/simplexml/tests/012.phpt Outdated
kocsismate
kocsismate commented Aug 24, 2020
View reviewed changes
Comment threadext/simplexml/simplexml.c Outdated
nikic
nikic reviewed Aug 25, 2020
View reviewed changes
Comment threadext/simplexml/simplexml.c Outdated
Comment threadext/simplexml/simplexml.c Outdated
nikic
nikic approved these changes Aug 25, 2020
View reviewed changes
Comment threadext/simplexml/simplexml.c
* and this is during runtime.
*/
zend_throw_error(NULL, "Cannot create unnamed attribute");
zend_throw_error(NULL, "Cannot append to an attribute list");

@nikicnikicAug 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave only the first line of the preceding comment.

@kocsismate

kocsismate commented Aug 25, 2020

Copy link
Copy Markdown
MemberAuthor

ext/simplexml/tests/bug37076_1.phpt fails because there's a memory leak somewhere - it seems to me that the new exception revealed it :S

kocsismate
kocsismate commented Aug 25, 2020
View reviewed changes
Comment threadZend/zend_object_handlers.h
* * &EG(error_zval), if an exception has been thrown.
* * NULL, if acquiring a direct pointer is not possible.
* In this case, the VM will fall back to using read_property and write_property.
*/

@kocsismatekocsismateAug 25, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the this explanation, it's really useful! Of course, I'm grateful for the fix, too :)

@kocsismate
kocsismate deleted the simplexml-warning branch August 25, 2020 13:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@nikicnikicnikic approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Warning promotion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kocsismate@nikic