DeuxHuitHuit / entry_relationship_field

A new way to create master-details pattern with Symphony's sections
http://symphonyextensions.com/extensions/entry_relationship_field/
Other
11 stars 12 forks source link

No longer selecting `*` elements in 2.0.1 #72

Closed andyjones closed 6 years ago

andyjones commented 6 years ago

We have many entry relationship fields that have Include elements in Data Sources and Backend Templates set to * expecting it to fetch all elements. I've updated from 2.0.0 to 2.0.1 and these elements are no longer included in the data source output.

It looks like it has something to do with the fetchIncludeableElements() changes introduced in https://github.com/DeuxHuitHuit/entry_relationship_field/commit/23d1c0bdc96a2ff3472678cba066d89dbbbd7739#diff-74f76b2010f612ced4f5aaa77da364f8R530

Is this the expected behaviour?

andyjones commented 6 years ago

I've updated from 2.0.0 to 2.0.1

whoops ignore that bit, it also happens in 2.0.0.

It returns the elements when I use fetchIncludeableElements() before https://github.com/DeuxHuitHuit/entry_relationship_field/commit/23d1c0bdc96a2ff3472678cba066d89dbbbd7739 and stops working after

nitriques commented 6 years ago

@andyjones

Can you copy the generated xml usign the ?debug flag please ?

andyjones commented 6 years ago

Hi! here is the debug xml (I've snipped out the other data sources to reduce the size a bit): https://gist.github.com/andyjones/ad6807d36086297c00fce6581c6c2f1a

The working version has entry ID 2039 that has an ERF entry ID 2040. Before the change to fetchIncludeableElements(), the debug xml includes 2040 in the output:

<sections entries="3708,3758,2022,3720,2039,2034,2037,3709" sections="...." ds-mode="*">
    <item id="2039" section-id="92" section="section" matched-element="*">
        <background-colour>
            <item id="1888" handle="yellow" section-handle="colours" section-name="Colours">yellow</item>
        </background-colour>
        <quotes entries="2040" sections="93" ds-mode="*">
            <item id="2040" section-id="93" section="quote" matched-element="*">
            </item>
        </quotes>
    </item>
</sections>

Since the change, entry id 2040 is no longer included:

<sections entries="3708,3758,2022,3720,2039,2034,2037,3709" sections="..."  x-data-source-mode="*" x-field-included-elements="*">
    <item id="2039" section-id="92" section="section" matched-element="*">
        </quotes>

    <item id="2039" level="1" max-level="-1" section-id="92" section="section" matched-element="*">
        <background-colour>
            <item id="1888" handle="yellow" section-handle="colours" section-name="Colours">yellow</item>
        </background-colour>
    </item>
</sections>
andyjones commented 6 years ago

Previously field.entry_relationship.php was doing:

if ($field instanceof FieldEntry_relationship) {
        $fieldIncludableElements = null;
}
if (!empty($fieldIncludableElements) && count($fieldIncludableElements) > 1) {
        // append each includable element
        foreach ($fieldIncludableElements as $fieldIncludableElement) {
                // remove field name from mode
                $submode = preg_replace('/^' . $fieldName . '\s*\:\s*/i', '', $fieldIncludableElement, 1);
                $field->appendFormattedElement($item, $data, $encode, $submode, $entry_id);
        }
} else {
        $field->appendFormattedElement($item, $data, $encode, $curMode, $entry_id);
}

$field is an instance of FieldEntry_relationship so we fall into the else block and call appendFormattedElement with $curMode=* to add the sub entries to the XML

andyjones commented 6 years ago

In contrast, the current version does:

if ($submodes === null) {
        if ($field instanceof FieldEntry_Relationship) {
// we are a FieldEntry_Relationship field so set `expandIncludableElements` to false
                $field->expandIncludableElements = false;
        }
// fetchIncludableElements() returns an empty array when `expandIncludableElements`
// is false so $submodes is an empty array
        $submodes = array_map(function ($fieldIncludableElement) use ($fieldName) {
                return FieldEntry_relationship::extractMode($fieldName, $fieldIncludableElement);
        }, $field->fetchIncludableElements());
        if ($field instanceof FieldEntry_Relationship) {
                $field->expandIncludableElements = true;
        }
}

// Append the formatted element for each requested mode
// but `$submodes` is an empty array so the sub entries are skipped
foreach ($submodes as $submode) {
        $field->appendFormattedElement($item, $data, $encode, $submode, $eId);
}

fetchIncludableElements returns an empty array so $submodes is empty as well:

// $this->expandIncludableElements was set to false earlier
public function fetchIncludableElements()
{
        $label = $this->get('element_name');
// $elements = array('*');
        $elements = $this->getArray('elements');
        $includedElements = array();
        if ($this->expandIncludableElements) {
// expandIncludableElements=false so never goes in here
                $includedElements[] = $label . ': *';
        }
        foreach ($elements as $elem) {
// $elem = '*'
                $elem = trim($elem);
                if ($elem !== '*') {
// $elem = '*' so doesn't go in here
                        $includedElements[] = $label . ': ' . $elem;
                } else if ($this->expandIncludableElements) {
// expandIncludableElements=false so never goes in here
                        break;
                }
        }
        if (empty($elements) && $this->expandIncludableElements) {
// expandIncludableElements=false so never goes in here
                $includedElements = array_unique(array_merge($includedElements, $this->fetchAllIncludableElements()));
        }
// returns an empty array
        return $includedElements;
}
nitriques commented 6 years ago

Wow @andyjones thanks for the detailed explanation. The change in behavior is not expected, I'll try to replicate it ASAP.

nitriques commented 6 years ago

Running 2.0.1 with this field

image

In the xml I get <content entries="1,487,346,166" sections="2,1,42,43" x-data-source-mode="*" x-field-included-elements="*"> with all the data in there.

There seems to be invalid xml in your comment, is it a copy paste error ?

andyjones commented 6 years ago

There seems to be invalid xml in your comment, is it a copy paste error ?

yes sorry, I messed the xml up when I was snipping bits out:

<page-content>
    <section id="2" handle="pages">Pages</section>
    <entry id="3694">
        <sections entries="3708,3758,2022,3720,2039,2034,2037,3709" sections="94,96,110,66,100,107,99,86,70,85,115,88,119,98,109,111,104,101,79,113,83,90,95,69,80,117,118,74,121,102,73,92,76,77,84,78,114,72,82,68,71,75,67,97,106,112,116" x-data-source-mode="*" x-field-included-elements="*">
            <item id="2039" level="1" max-level="-1" section-id="92" section="section" matched-element="*">
                <background-colour>
                    <item id="1888" handle="yellow" section-handle="colours" section-name="Colours">yellow</item>
                </background-colour>
            </item>
        </sections>
    </entry>
</page-content>
andyjones commented 6 years ago

I think it only affects ERF entries that are nested in another ERF entry. In the example above, <sections> and <item> are ERFs. It is fetching the entries for the top level ERF but not the nested <item> ERF.

I'll try to get a repro.

andyjones commented 6 years ago

I'm able to reproduce with these steps:

  1. create a new section that only contains a text field - I've given it a name and handle of text-section:

image

  1. create a new section that only contains an ERF field, available sections = the text section just created, include elements = '*'. I've named it nested:

image

  1. create a new top level section that only contains an ERF field, available sections = the nested section just created, include elements = '*'. I've named it top:

image

  1. Create a new entry for the top level section (you will have to create a new entry for the nested and text fields too)

When I view the debug XML for this entry, it selects the entries belonging to top but not nested.

In case it is useful, I'm now getting the debug xml with https://gist.github.com/andyjones/d0b03cd85ad7b70e84f357bb8f320d65 so I don't have to attach the entry to a page.

nitriques commented 6 years ago

Hi @andyjones sorry for the late reply, been pretty busy. Can you tell me if 8c2e380 fixes the problems on your end ? Thanks again for all the precious information you shared, help me fix the problem !

andyjones commented 6 years ago

Works great

Thanks for fixing, appreciate it :)

nitriques commented 6 years ago

Awesome. The most complicated thing was already done by you, the hardest thing was to not break anything. The fix was so simple to implement.

I'll release 2.0.2 right now. Please open other issues if you encounter anything weird!