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:
- Standard Drupal 8 install
- Enabled Aggregator Module
- Edit "Content" view (or any other): admin/structure/views/view/content
- Click "Add" on Header or Footer in view configuration
- Select "Global: Rendered entity - Aggregator feed item" and "Apply"
- Note Configuration for Aggregator feed item is not presented, Above error should appear in logs
- 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.
Comment | File | Size | Author |
---|---|---|---|
#34 | 2559529-34.patch | 1.63 KB | dawehner |
#28 | aggregator_item_area-2559529-28.patch | 1.56 KB | Lendude |
#28 | interdiff-2559529-26-28.txt | 604 bytes | Lendude |
#26 | aggregator_item_area-2559529-26.patch | 845 bytes | Lendude |
#18 | aggregator_item_area-2559529-18.patch | 4.2 KB | Lendude |
Comments
Comment #2
justAChris CreditAttribution: justAChris as a volunteer commentedAdded steps to reproduce.
Updating this to major since:
https://www.drupal.org/core/issue-priority
Comment #3
geertvd CreditAttribution: geertvd at XIO commentedThis seems related #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field
Comment #4
geertvd CreditAttribution: geertvd at XIO commentedI 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?
Comment #5
geertvd CreditAttribution: geertvd at XIO commentedSo 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.
Comment #6
LendudeTest 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).Comment #9
LendudeThis should clear up some fails I think (probably not all).
Comment #11
LendudeScratch 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.
Comment #13
LendudeFixed config.
Comment #14
xjmThanks @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.
Comment #15
xjmComment #16
dawehnerCan'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.
Comment #17
xjmThe 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.
Comment #18
Lendude@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.
Comment #20
LendudeHad a bit of a think about this and:
Per @xjm in #14:
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.
Comment #21
LendudeComment #22
dawehnerGiven that UUIDs are optional the fix for this issue is fundamental wrong ... and maybe we could move that to its own issue.
Comment #26
LendudeOk, 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?
Comment #28
LendudeWell, that fail actually gives us test coverage for the modified logic....
Comment #34
dawehnerI was wondering whether some UI tests would be helpful here.
Comment #35
dawehnerThis should definitively fail.