Updated: Comment #0

Problem/Motivation

DateFormatAccessController and NodeTypeAccessController relies on isLocked() method of the entity to prevent delete entities that are used by enabled modules. Menu entity relies on menu_list_system_menus() to prevent delete system menus but this going to change #2084197: Uninstalling menu module removes all custom menus in favour of locked property on entity.
So currently there's no common approach to prevent some config to be deleted.

Locking usage

Proposed resolution

Implement a service to allow modules to lock entity definitions (probably config files) that are required them to operate properly.
For example: forum relies on vocabulary and node type that are required to work.

Remaining tasks

tbd

User interface changes

no

API changes

tbd

#2084197: Uninstalling menu module removes all custom menus
#2003892: Convert date formats to config entities
#111715: Convert node/content types into configuration

CommentFileSizeAuthor
#9 2084567-9.patch23.9 KBtstoeckler
#9 2084567-8-9-interdiff.txt13.61 KBtstoeckler
#8 2084567-7.patch19.45 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

The language entities also have a concept of locked and will not even list locked entities in the list controller. Would be a good one to add on this list :)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

added links to types

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

Updated issue summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

Updated issue summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Title: Implement a service to allow locking entities » Implement a EntityStateInterface and service to allow locking entities

@Gabor, yes, the same interface needed for language types in #1862202-157: Objectify the language system

probably better title according #2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

markup

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
Issue tags: -entity API +Entity Field API
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Assigned: Unassigned » tstoeckler

Taking a stab at this.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

This makes sense, however I think the name EntityStateInterface is too generic. Maybe, EntityLockInterface would describe it better?

Also, at discussed over at #2031717-15: Make entity module not required it might be worthwhile to have CMI enforce the locking.

The way this works with entity exportables in d7 is that deletion is actually a revertion - so I guess this is what people would expect to happen? However, issues through the delete+insert approach occurred as the deletion reactions are often unwanted in the revertion case, so a dedicated revert hook would be a a good improvement.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

I also did not like EntityStateInterface. To be consistent with EntityChangedInterface I went with EntityLockedInterface for now. (Will upload patch soon.) Re-titling for that.

I think a dedicated 'revert' operation + hook, etc. makes sense, but if I'm not mistaken that could be a follow-up right?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Title: Implement a EntityStateInterface and service to allow locking entities » Implement a EntityLockedInterface and service to allow locking entities
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review
FileSize
2084567-7.patch19.45 KB

So we have two different usages of locked currently:

Date formats and languages
  • Locked entities cannot be deleted or updated
  • Locked date formats are not shown in the list controller
Node types and menus
  • Locked entities cannot be deleted

This patch implements the latter behavior for EntityLockedInterface and keeps the custom implementations of Language and DateFormat on top of that. We should probably find a cleaner way to do that and/or better terminology.

To be more specific on top of providing the EntityLockedInterface the patch does two things:

  1. Implement an access check in EntityAccessController to prevent locked entities from being deleted
  2. Add the 'locked' property of lockable entities to getExportProperties() automatically so that $locked can have protected visibility but still be exported automatically.

This patch in-passing also fixes a minor bug where currently DateFormatAccessController does not check create access. By supplying an 'admin_permission' in the annotation this is done automatically.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2084567-8-9-interdiff.txt13.61 KB
2084567-9.patch23.9 KB

The patch was missing the isLocked() method in the Language entity class.

From looking around core I am now prettty convinced that we need to clarify the terminology here. The word 'lock' is already very ambiguous. On top the usage that is discussed here, we already have the whole locking system á la LockInterface / lock.inc and also views can be locked from editing but only if another person is currently editing it.

Therefore in this patch I introduce:
1. EntityDeletionLockedInterface
2. EntityUpdateLockedInterface
(Please *do* provide suggestions for better names.)

With this the access control of date formats, languages, menus and node types are all covered. Only the list controller magic of date formats in languages mentioned in #8 is still done manually.

Status: Needs review » Needs work

The last submitted patch, 9: 2084567-9.patch, failed testing.

The last submitted patch, 8: 2084567-7.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Parent issue: » #111715: Convert node/content types into configuration
Related issues: +#1839516: Introduce entity operation providers, +#1965910: Remove enable/disable entity operations, +#2084197: Uninstalling menu module removes all custom menus, +#2003892: Convert date formats to config entities

I think #9 is overcomplicated but this points why I used 'state' in issue.
State is not boolean, I suppose a set of disabled operations better describes this interface.
So probably better to move the implementation to operations level after #1839516: Introduce entity operation providers and related #1965910: Remove enable/disable entity operations

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -176,6 +178,20 @@ public function getExportProperties() {
+    // Export the 'locked' property if the entity supports locking.
+    if ($class_info->getProperty('locked')->isProtected()) {
+      if ($this instanceof EntityDeletionLockedInterface) {
+        $properties['locked'] = $this->isLockedFromDeletion();
+      }
+      // In case the entity supports both update and deletion locking this
+      // assumes that both are handled by a single 'locked' property. If the
+      // locking of updating and deletion are to be handled independently, this
+      // behavior should be overridden in the child implementation of this
+      // method.
+      elseif ($this instanceof EntityUpdateLockedInterface) {
+        $properties['locked'] = $this->isLockedFromUpdate();

looks fragile

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

I'm fine with going back to the approach in #8.

As I've tried to explain in #9 what we currently have in core is confusing when you look closely. Regardless of the other usages of the word 'lock' in core currently, DateFormatInterface::isLocked() has a different meaning that NodeType::isLocked(). That's the problem that I was trying to solve.

Regarding the hunk quoted in #12: I'm fine with removing that for now. I just thought it would be handy not to have to redeclare 'locked' in getExportProperties the whole time.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Assigned: tstoeckler » Unassigned

So this issue really needs some more discussion input before progressing with this. Unassigning for now. Maybe someone can summon the Entity API gurus :-)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: +API addition

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.9.x-dev » 9.2.x-dev
fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm as a volunteer and at OSInet commented

FWIW, I have this issue in the G2 module, to prevent deletion of a specific vocabulary and misc fields, similar to Forum.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented

Mused a bit about this. The problem eith the "locked" property altogether is, that is has no info why something is locked.
Think 2 components set a locked property. One does not need the lock anymore, not knowing that the other still needs the lock.
Also we can never know why something is locked, nor who set the property. Bad.

So what about kill the locked property in favor of (something like) hook_entity_change_veto()?
We can invoke that hook and display to the user something like: "Deleting this entity is disallowed by module Foo because 'foo reason'", "Editing or deleting this entity is disallowed by module Bar because 'bar reason'".

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Meantime here's core enforced dependencies for confirmation, also transform and override different kinds.

Thinking about this interface for content I think itl may affect default content (making it read only)

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented

@andipost #27: I read that several times and do not get what you want to say...

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.