craftcms / feed-me

Craft CMS plugin for importing entry data from XML, RSS or ATOM feeds—routine task or on-demand.
Other
288 stars 139 forks source link

"Disable globally" not working if element isn't in primary site #1468

Closed tricki closed 2 months ago

tricki commented 3 months ago

Description

We have an existing import to a non-primary site, which is working fine, except for disabling the elements that no longer exist in the import ("Disable missing elements globally"). In the log, in the last step, there's always this error: Attempt to read property "enabled" on null.

image

The error occurs on this line: https://github.com/craftcms/feed-me/blob/5.5.0/src/base/Element.php#L215

That method:

public function disable($elementIds): bool
{
    /** @var CraftElementInterface|string $class */
    $class = $this->getElementClass();
    $elementsService = Craft::$app->getElements();

    foreach ($elementIds as $elementId) {
        /** @var BaseElement $element */
        $element = $elementsService->getElementById($elementId, $class);
        if ($element->enabled) {
            $element->enabled = false;
            $elementsService->saveElement($element, true, true, Hash::get($this->feed, 'updateSearchIndexes'));
        }
    }

    return true;
}

This is trying to get the element to disable it. But because the $siteId isn't passed to getElementById (the third argument), it tries to find it in the primary site. But the problem is the elements we're importing aren't in the primary site.

I fixed it by passing "*" as the site id. Since the feed option includes the word "globally", and the same class has another method called disableForSite(), I think "*" is the right choice.

- $element = $elementsService->getElementById($elementId, $class);
+ $element = $elementsService->getElementById($elementId, $class, '*');

Steps to reproduce

  1. Create a new site with at least one element type
  2. Create an import for that site and entry type with "Disable missing elements globally" enabled.
  3. Manually create an entry
  4. Run an empty import
  5. The entry should get disabled, instead there's an error.

Additional info

i-just commented 3 months ago

Hi, thanks for reporting and the very detailed description - much appreciated! I raised a PR for this.

tricki commented 2 months ago

Thanks for the quick fix. I just tested 5.x-dev and can confirm that disabling now works.