Clean up todo in InlineBlockEntityOperations::handleEntityDelete()

I wonder what's the use case for validating an entity after it has been deleted from the storage. The issue title says that the entity is being deleted, but that's not really accurate, the entity is already gone.

I think the test could benefit from a comment explaining why we're calling validate(), but otherwise this looks good and is ready to go.

I like it. RTBC on the assumption that it will look like Christmas in an hour.

I'd like to take another look at the patch as well, hopefully there's no rush to commit it..

I've been staring for a while at \Drupal\layout_builder\InlineBlockEntityOperations::handleEntityDelete() and all its possible code paths, and I can't find what would trigger a call to EntityContext::fromEntity($entity) in there..

Is there some other part of the code that I should also be looking at? :)

Oops, didn't want to un-assign, I'm still looking into it.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
@@ -124,7 +124,7 @@ public function validate($value, Constraint $constraint) {
-        $existing_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id());
+        $existing_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id()) ?: $entity;

This looks very reasonable - but perhaps a code comment might be a good idea - so people know when we might use the passed in entity instead of the unchanged one.

Setting to NW for the code comment mentioned in #16.

Tagging to get some eyes to add the comment

Updated patch with reroll diff & addressed #16

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Adding patch against 10.0.1

Did a brief code review and the following comment doesn't make sense to me, I think we should reword it. Otherwise, the code looks good
// We might use the passed entity instead of the unchanged one.

Made change to the comment as per #30 so the comment is now quite relevant what it supposed to do.

Attempted to replicate this by running the code in the issue summary and I don't get any error in 10.1

Can someone please confirm if it's still an issue and update the issue summary with additional steps

Thanks!

There's still a reference in code to this issue in \Drupal\layout_builder\InlineBlockEntityOperations::handleEntityDelete(), let's see if calling isLayoutCompatibleEntity() is possible now.

Opened a MR, let's see how it goes.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding the patches because the MR seems to work well :)

Updated title and solution to mention this seems to be cleaning up a todo and use isLayoutCompatibleEntity().

Added a question the MR.

Moving to NW for someone to test that proposal.

Hi all,
I removed the isLayoutCompatibleEntity($entity) check before calling removeByLayoutEntity($entity), as suggested.

Tests performed:

PHPUnit: All tests passed successfully, with no errors found. Only pre-existing skipped tests remained unchanged.

The change has been committed and submitted in the Merge Request. Awaiting feedback for any necessary adjustments!

Thank you!

MR is 1800+ commits back.

Hi @smustgrave

I have successfully rebased the branch with the latest changes from 11.x. The MR !7106 is now up to date, and the previous changes remain intact.

Additionally, I noticed that one test failed, but it does not appear to be related to the changes made in this MR.

Please let me know if you need any additional adjustments.

Thanks!

So reading the issue summary and title

update InlineBlockEntityOperations::handleEntityDelete() to use isLayoutCompatibleEntity()

Still seems to be needed.

Hi @smustgrave,

Apologies if I might be misunderstanding something here, but I just want to clarify the intent behind this change to ensure I'm following the best approach. I'm still getting started with contributing to Drupal, and I really appreciate your guidance.

The issue summary suggests that handleEntityDelete() should be updated to use isLayoutCompatibleEntity(), but after reviewing the implementation, I noticed that removeByLayoutEntity($entity) does not fail if the entity is not layout-compatible. Instead, it simply attempts to clean up inline_block_usage, setting layout_entity_id and layout_entity_type to NULL if applicable. If no matching records exist, nothing happens, which makes me wonder if the extra check is truly necessary.

I just want to make sure I'm not missing something here:

• Was there a specific issue that required adding isLayoutCompatibleEntity()?
• Is there any scenario where calling removeByLayoutEntity() directly could cause unintended side effects?

If the check is needed for a reason I’m not seeing, I’ll be happy to update it accordingly. Otherwise, removing it might help simplify the logic while still ensuring the cleanup happens.

Again, thanks for your patience and feedback—I really appreciate learning from this process!

No worries. So might help to look at the reference issue this was created from.

Not a layout builder expert to say what’s needed or not but the summary needs to match the MR. Helps committers quickly understand a ticket

If it’s determined nothing is needed then the summary could include your findings and just a simple solution of remove the todo

Can put back into review for more eyes but before RTBC summary will have to match

Thanks. then believe this ticket may be ready.

Thanks @smustgrave and @brandonlira for digging into the issue. The issue summary was indeed out of date, as the scope of this issue changed when old code was removed. See #34 and #35.

Since a field subsystem maintainer created this MR, that gives me confidence that this @todo is invalid and should be removed.

Saving issue credits.

Since it's a non-disruptive cleanup of bad, actually wrong inline docs, I backported it all the way to 10.5.x. (Branch parity for things like tests and docs is sort of a gray area for maintenance minors; it's not officially listed but for some things we can keep parity.)

Thanks everyone!