Problem/Motivation

I'm unable to add an "Aggregator feed item" area to the footer of a view. The settings of the area handler don't appear.

The following error appears in watchdog:
Drupal\Core\Entity\EntityStorageException: Entity type aggregator_item does not support UUIDs. in Drupal\Core\Entity\EntityManager->loadEntityByUuid() (line 1132 of /home/dfg64/www/core/lib/Drupal/Core/Entity/EntityManager.php).

Steps to reproduce:

  1. Standard Drupal 8 install
  2. Enabled Aggregator Module
  3. Edit "Content" view (or any other): admin/structure/views/view/content
  4. Click "Add" on Header or Footer in view configuration
  5. Select "Global: Rendered entity - Aggregator feed item" and "Apply"
  6. Note Configuration for Aggregator feed item is not presented, Above error should appear in logs
  7. Closing the "Add" Dialog and clicking "Save" on view will also produce this error.

Proposed resolution

Fix it. Write tests.

Remaining tasks

Fix it. Write tests.

Comments

geertvd created an issue. See original summary.

justAChris’s picture

Priority: Normal » Major
Issue summary: View changes

Added steps to reproduce.

Updating this to major since:

Examples of major bugs:

A non-fatal PHP error, or a PHP error triggered under rare circumstances or affecting only a small percentage of all users

https://www.drupal.org/core/issue-priority

geertvd’s picture

geertvd’s picture

I also don't know if it makes sense to have a single rendered aggregator feed item. Wouldn't it make more sense not to have handler for this entity type?

geertvd’s picture

So to fix this we need to give aggregator feed items a uuid, I'm not sure why they don't have one at the moment but this kinda feels like overkill here.
Not sure how to exclude it either though.

Lendude’s picture

Test and a fix. Not sure at all if this is the right fix. Looking at the discussion in #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field it would seem other people feel that the guid is the equivalent of UUID in an aggregator setting, so it would make sense to just set it up like that.

It just seems that most of the code surrounding this is expecting all content entities to have a uuid setting, I don't know if that is actually a requirement of if it's just true for nearly all content entities. But other options I see would be to either wrap all calls to UUID based logic in a if (!$uuid_key = $entity_type->getKey('uuid')) { type if (did a quick test with this and just lead to more errors getting thrown), or add a real UUID field to the aggregator item entity as discussed in #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field (and dismissed there).

The last submitted patch, 6: aggregator_item_area-2559529-6-TEST_ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: aggregator_item_area-2559529-6.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
5.22 KB

This should clear up some fails I think (probably not all).

Status: Needs review » Needs work

The last submitted patch, 9: aggregator_item_area-2559529-9.patch, failed testing.

Lendude’s picture

Scratch that uuid stuff, easier fix, lets see what this does. No interdiff cause this is a completely new approach.

Just make the code work for content entities with no uuid.

Status: Needs review » Needs work

The last submitted patch, 11: aggregator_item_area-2559529-11.patch, failed testing.

Lendude’s picture

xjm’s picture

Title: Unable to add "Aggregator feed item"-area to a view region » Adding "Aggregator feed item"-area to a view region fails with exception due to config dependency target bug
Status: Needs review » Needs work
Issue tags: +Configuration system, +config dependency
Related issues: +#2341357: Views entity area config is not deployable and missing dependencies

Thanks @geertvd for reporting this and @Lendude for working on the fix!

So the original issue in #2341357: Views entity area config is not deployable and missing dependencies made an (apparently incorrect) assumption that all content entities would have UUID keys. #13 tries to correct that bad assumption.

However, with #13, we are still in a situation where the entity area handler for the aggregator feed item is not deployable, because the serial ID of '1' can easily be different between two environments.

So I think the best fix might actually be to add UUIDs for these entities, along the lines of the earlier approach. We should also discuss what to do in other cases where the UUID is missing, though.

xjm’s picture

dawehner’s picture

So I think the best fix might actually be to add UUIDs for these entities, along the lines of the earlier approach.

Can't agree more. For me UUIDs are a requirement for entities just like IDs itself. I bet there is a lot of code out there which assumes that UUIDs are available. Maybe we should actually validate it somewhere.

xjm’s picture

Issue tags: +Triaged D8 major

The core committers and Views maintainers (alexpott, xjm, effulgentsia, tim.plunkett, and dawehner) agreed that this is a major issue. It both causes an exception and makes these aggregator area plugins not deployable. It's not critical because it is a pretty specific usecase.

Lendude’s picture

@xjm and @dawehner thanks for the feedback!

Ok back to the UUID approach, so this is a continuation of #9. Bit of a cleanup. And hopefully this will take care of the failures in #9.

Status: Needs review » Needs work

The last submitted patch, 18: aggregator_item_area-2559529-18.patch, failed testing.

Lendude’s picture

Title: Adding "Aggregator feed item"-area to a view region fails with exception due to config dependency target bug » Adding 'Rendered entity' to a view region when the entity does not have a UUID fails with exception due to config dependency target bug
Component: aggregator.module » views.module

Had a bit of a think about this and:

Per @xjm in #14:

We should also discuss what to do in other cases where the UUID is missing, though.

Either UUIDs are required or they are not. I think we all agree on the fact that they should be required, but if the current entity api doesn't require them (it doesn't now right?), then Views needs a way to deal with non-UUID content entities (even if all core entities have one, contrib might not, since it's not required). That would be the patch in #13.

Adding a UUID to all core entities should be separate from this, for the aggregator item there is already an issue for this #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field, so I'm hijacking this issue to focus on the Views side of this.

If, on the other hand, we decide that UUIDs are required, then from the Views side of this, we don't need to do anything and the code works as is and we can just close this issue (but I don't think that is the case).

Setting to 'Needs review' for patch in #13 which I think is the way to go until UUIDs are required.

Lendude’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work

Given that UUIDs are optional the fix for this issue is fundamental wrong ... and maybe we could move that to its own issue.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 13: aggregator_item_area-2559529-13.patch, failed testing.

The last submitted patch, 13: aggregator_item_area-2559529-13.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
845 bytes

Ok, another new approach that I discussed shortly with @dawehner on IRC: keep this within Views.

So how about: Views only supports rendered entities with a UUID.

This would obviously still need tests and maybe an upgrade path (but everything that would try to do this right now would just throw an error and not be saved so is that really needed)? But is this a direction that we can get behind as a fix for Views?

Status: Needs review » Needs work

The last submitted patch, 26: aggregator_item_area-2559529-26.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
604 bytes
1.56 KB

Well, that fail actually gives us test coverage for the modified logic....

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.