In the Paragraphs access check we give access to the paragraph depending on the access of the parent entity. However when fetching the parent entity it will always return the entity in the original language. This means that If a user don't have access to the original parent entity, he will not have access to the paragraph in any other language either.

Proposed fix:

Add code that returns the translated parent entity instead, if there is one.

Here is quick patch. There is probably a much cleaner way of doing this though :)

CommentFileSizeAuthor
#13 getparententity_does-2790613-13.patch3.29 KBjohnchque
#13 getparententity_does-2790613-13-test-only.patch1.96 KBjohnchque
#8 load_translated_parent_entity_2.patch1.33 KBreekris
load_translated_parent_entity.patch942 bytesreekris
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

reekris created an issue. See original summary.

reekris’s picture

reekris CreditAttribution: reekris commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, load_translated_parent_entity.patch, failed testing.

The last submitted patch, load_translated_parent_entity.patch, failed testing.

The last submitted patch, load_translated_parent_entity.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented

Indeed, the user should be able to get the parent in the correct language.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented 
+++ b/src/Entity/Paragraph.php
@@ -84,7 +84,15 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
+    if ($parent != NULL && method_exists($parent, 'hasTranslation') && $parent->hasTranslation($this->language()->getId())) {

I think we should properly check for TranslatableInterface.

reekris’s picture

reekris CreditAttribution: reekris commented
FileSize
load_translated_parent_entity_2.patch1.33 KB

Yes, that's a nicer way of checking for translatability. Here is an updated patch.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review

Let's see if this breaks something, if not then is time to add tests. :)

Status: Needs review » Needs work

The last submitted patch, 8: load_translated_parent_entity_2.patch, failed testing.

The last submitted patch, 8: load_translated_parent_entity_2.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented

Seems to work, now we just need tests. Test fail is unrelated.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
getparententity_does-2790613-13-test-only.patch1.96 KB
getparententity_does-2790613-13.patch3.29 KB

Tests added. Interdiff = Test only.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: getparententity_does-2790613-13.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review

Unrelated test fail.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

jmuzz’s picture

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

Seems solid. The test only passes with the change to the entity on my installation too.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

The last submitted patch, 13: getparententity_does-2790613-13-test-only.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented

Seems this is still applying.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Committed.

Also added some comment on getParentEntity documentation.

Status: Fixed » Closed (fixed)

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

james.williams’s picture

james.williams CreditAttribution: james.williams at ComputerMinds commented

Sorry to post on such an old issue here -- but #2854867: getParentEntity() does not respect translation fallbacks is a follow-up to this, and @Berdir has suggested that the Entity Repository service might be better to use, rather than directly loading & getting the translation. Was that considered for this work, and/or are there any good reasons not to?