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

EZP-28118: Error in RelationConverter on content after editing class in legacy #2115

Merged
merged 3 commits into from Oct 24, 2017

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Oct 20, 2017

Issue: https://jira.ez.no/browse/EZP-28118
Regression from #2022
Discover on ezsystems/ezpublish-legacy#1201 (comment)

Todo:

  • adjust test

@andrerom andrerom changed the base branch from master to 6.11 October 20, 2017 13:34
@andrerom andrerom changed the title EZP-28118: Error in RelationConverter on content after editing relati… EZP-28118: Error in RelationConverter on content after editing class in legacy Oct 20, 2017
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Make sense 👍

if (
($selectionType = $dom->getElementsByTagName('selection_type')) &&
$selectionType->item(0)->hasAttribute('value')
) {
$fieldSettings['selectionMethod'] = (int)$selectionType->item(0)->getAttribute('value');
Copy link
Member

Choose a reason for hiding this comment

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

Quick test showed me that getElementsByTagName never returns an empty value. It's always \DOMNodeList. It has property length. If length = 0, then $selectionType->item(0) will return null and calling hasAttribute will throw an exception.
So rather:
if (($selectionTypeFirstItem = $dom->getElementsByTagName('selection_type')->item(0)) && $selectionTypeFirstItem->hasAttribute('value'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so DomList never evaluates to empty here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Consider the following example (just re-pasting from elsewhere to keep discussion here consistent):

$xml = <<< XML
<?xml version="1.0" encoding="utf-8"?>
<para></para>
XML;

$dom = new DOMDocument;
$dom->loadXML($xml);
if (($elements = $dom->getElementsByTagName('non_existent_tag'))) {
	var_dump($elements->item(0));
}

will output NULL.

if (
($selectionType = $dom->getElementsByTagName('selection_type')) &&
$selectionType->item(0)->hasAttribute('value')
) {
Copy link
Member

Choose a reason for hiding this comment

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

Same remark as above

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Looks good :)

@andrerom andrerom merged commit 3cbd180 into 6.11 Oct 24, 2017
@andrerom andrerom deleted the EZP-28118 branch October 24, 2017 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants