The Quickedit module calls $entity->isLatestRevision() several places on an object that type is \Drupal\Core\Entity\EntityInterface without checking whether the object also implements the \Drupal\Core\Entity\RevisionableInterface interface.

CommentFileSizeAuthor
#18 3045384-18.patch2.53 KByogeshmpawar
#7 interdiff_3_6.txt650 bytesmxr576
#6 interdiff_6_3.txt650 bytesmxr576
#6 quickedit-revisioning-fix-3045384-6.patch2.56 KBmxr576
#3 quickedit-revisioning-fix-3045384-3.patch2.51 KBmxr576
#2 quickedit-revisioning-fix-3045384-2.patch2.51 KBmxr576

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Title: Not all entity is revisionable » Fix incorrect assumption that all entity are revisionable
Status: Active » Needs review
StatusFileSize
new quickedit-revisioning-fix-3045384-2.patch2.51 KB
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new quickedit-revisioning-fix-3045384-3.patch2.51 KB
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Version: 8.6.x-dev » 8.7.x-dev
amateescu’s picture

amateescu commented
Status: Needs review » Needs work 
+++ b/core/modules/quickedit/quickedit.module
@@ -11,9 +11,9 @@
-use Drupal\Core\Entity\ContentEntityInterface;
...
+use Drupal\Core\Entity\RevisionableInterface;

@@ -169,7 +169,7 @@ function quickedit_entity_view_alter(&$build, EntityInterface $entity, EntityVie
- * @param \Drupal\Core\Entity\ContentEntityInterface $entity
+ * @param \Drupal\Core\Entity\RevisionableInterface $entity

@@ -182,7 +182,7 @@ function quickedit_entity_view_alter(&$build, EntityInterface $entity, EntityVie
-function _quickedit_entity_is_latest_revision(ContentEntityInterface $entity) {
+function _quickedit_entity_is_latest_revision(RevisionableInterface $entity) {

These changes are not necessary because ContentEntityInterface extends RevisionableInterface, so let's remove them from the patch.

The other two changes look good :)

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review
StatusFileSize
new quickedit-revisioning-fix-3045384-6.patch2.56 KB
new interdiff_6_3.txt650 bytes

Thanks for the review @amateescu.

I think both are important because both functions should support all revisionable entity types - not just content entities. Developers who are modifying the body of this function should not expect that $entity implements the ContentEntityInterface. They should expect what it is in the parameter type hint, EntityInterface, and if the object also implements the RevisionableInterface then they can also call methods defined by that interface.

So as a conclusion I have removed the extra type-hint from quickedit_entity_view_alter().

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new interdiff_3_6.txt650 bytes

Meh :)

amateescu’s picture

amateescu commented

Yup, I agree that we need the changes in quickedit_preprocess_field() and quickedit_entity_view_alter, but #5 was about rolling back the changes to _quickedit_entity_is_latest_revision() :)

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

I got it but I think that function should not type-hint to ContentEntityInterface either because of the above-mentioned reasons. It should type-hint to the less specific interface.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Ok then, let's do it.

catch’s picture

catch
he/him
Primary language English
commented

Does this need test coverage? If it's just clean-up/theoretical I think it's OK without, but if it's causing an error we'd be missing coverage.

amateescu’s picture

amateescu commented

The problem could only happen for an entity type that implements RevisionableInterface but not ContentEntityInterface, and we don't have any example of that in core, not even in a test entity type. Not sure if that makes it theoretical or not.. :)

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

#12 +1

Although, considering the long term run, Drupal core should have its own test entities later are only fieldable, or only revisionable, or both, but not content entities :)

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

The problem could only happen for an entity type that implements RevisionableInterface but not ContentEntityInterface, and we don't have any example of that in core, not even in a test entity type. Not sure if that makes it theoretical or not.. :)

I have an example for that in a contrib module that creates 1rst citizen Drupal entities from REST API responses. I am planning to write a blog post about our journey and all the challenges that we faced when we tried to build entities in Drupal 8 that are not content- or config entities. There were plenty... :) So I do not want to spoil anything by sharing its code here.

Can we get this merged?

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue tags: +not every entity is content or config
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll 
error: patch failed: core/modules/quickedit/quickedit.module:11
error: core/modules/quickedit/quickedit.module: patch does not apply
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3045384-18.patch2.53 KB

Rerolled the patch against 8.7.x branch.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Per the RTBC from @amateescu in #10, and the un-RTBC by @catch in #11 which was answered by @amateescu in #12, re-RTBC'ing.

  • catch committed 983b2a6 on 8.7.x 
    Issue #3045384 by mxr576, yogeshmpawar, amateescu: Fix incorrect...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.8.x and 8.7.x, thanks!

  • catch committed 2a0c532 on 8.8.x 
    Issue #3045384 by mxr576, yogeshmpawar, amateescu: Fix incorrect...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.