Problem/Motivation

I'm using InlineParagraphsWidget with a custom entity type, not Paragraphs. As soon as I create an entity I see Paragraph's You are not allowed to remove this Paragraph. message. This is because the access check ends up in \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess() which does:

    if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }

Since #2733535: Better Paragraph deletion permission logics, upon $parent_access this no longer happens for Paragraphs itself because that check is never reached. I could implement a similar access logic for my own entity type, but I think the logic in the widget is actually incorrect. delete is a storage-related operation so checking that for new entities is simply wrong, I think.

Proposed resolution

Check $entity->isNew() in addition to $entity->access('delete')

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

Berdir’s picture

Not sure I understand this 100% yet, but makes sense I think, patch? :)

tstoeckler’s picture

Status: Active » Needs review
FileSize
7.79 KB

Kind of lost track of this, here we go.

This replaces $paragraphs_entity->access('delete') with $paragraphs_entity->isNew() || $paragraphs_entity->access('delete') consistently in InlineParagrapsWidget and ParagraphsWidget (including some boolean logic simplification. I also removed one obsolete instantiation of the $button_access variable, since that seemed reasonably in scope since we're modifying that variable above.

This works for me and properly shows a Remove button for newly created paragraphs with my custom paragraphs entity type.

tstoeckler’s picture

FileSize
6.14 KB

Rerolled for 1.2

I decided to put the two delete-related checks in a variable as that makes things slightly more readable.

Status: Needs review » Needs work

The last submitted patch, 4: 2851492-4.patch, failed testing. View results

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
6.14 KB

Oh, so I guess 8.x-1.x is moving too fast for me ;-) This one should apply

tstoeckler’s picture

FileSize
5.58 KB

Needed a re-roll again.

Not sure if it's intentional that the new removeButtonAccess() is not used everywhere.

In any case would be great to get some feedback here, on what's needed to get this in.

Status: Needs review » Needs work

The last submitted patch, 7: 2851492-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
717 bytes
5.64 KB

Here we go, this should be green.

DigitalCatalyst’s picture

I checked out this patch, it does hide the message "You are not allowed to remove this Paragraph." on a paragraph, however, it reverts the hiding of the Remove button, for paragraphs when you have only 1 paragraph, from this thread, https://www.drupal.org/project/paragraphs/issues/2919186

gg24’s picture

Hi @DigitalCatalyst,

Yes, the patch works perfectly fine for me and it serves the purpose of this issue. The point raised that it reverts the hiding of the Remove button, for paragraphs when you have only 1 paragraph, from this thread, https://www.drupal.org/project/paragraphs/issues/2919186 is not related to this fix and here just the new entity check is added along with the delete access. I think we can move this to RTBC.

Thanks, @tstoeckler for the patch. Patch works as expected.
Hence moving it to RTBC.

Thanks!

gg24’s picture

Status: Needs review » Reviewed & tested by the community
Berdir’s picture

Yes, I don't see how this reverts the other issues, please provide steps to reproduce. We have test coverage for that and it doesn't fail.

  • Berdir committed 3ca7da1 on 8.x-1.x authored by tstoeckler
    Issue #2851492 by tstoeckler: Paragraphs should not check delete access...
Berdir’s picture

Status: Reviewed & tested by the community » Fixed

No feedback, so committed. If someone can reproduce, feel free to create a new issue.

Status: Fixed » Closed (fixed)

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