Problem/Motivation

Currently it is possible to activate the content lock only on bundle level, but it is useful to activate it on an entity level so that if there are new bundles added that they automatically have the content lock activated.

Proposed resolution

Add an "All bundles" to the configuration form and in \Drupal\content_lock\ContentLock\ContentLock::isLockable() check if this option is activated if the bundle is not selected.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#17 2911446-17.patch6.84 KBkfritsche
#17 2911446-interdiff-13-17.txt1.91 KBkfritsche
#13 2911446-13.patch5.39 KBkfritsche
#10 2911446-interdiff-8-10.txt1.49 KBkfritsche
#10 2911446-10.patch4.52 KBkfritsche
#8 2911446-8.patch4.42 KBkfritsche
#8 2911446-interdiff-7-8.txt2.11 KBkfritsche
#7 2911446-7.patch3.35 KBkfritsche
#7 2911446-interdiff-6-7.txt1.86 KBkfritsche
#6 interdiff-4-6.txt1.32 KBkfritsche
#6 2911446-6.patch3.19 KBkfritsche
#4 2911446-4.patch3.12 KBhchonov
#2 2911446-2.patch3.13 KBhchonov

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
StatusFileSize
new 2911446-2.patch3.13 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2911446-2.patch, failed testing. View results

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Needs work » Needs review
StatusFileSize
new 2911446-4.patch3.12 KB

Oups. The patch should apply now.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs review » Needs work

In general i like the idea. Could we improve the settings form usability a bit. I think it's confusing if you are able to check "All bundles" and additionally specific bundles. So i think we should hide all other bundles if "All bundles" is selected.

+++ b/js/content_lock_settings.js
@@ -0,0 +1,37 @@
+  Drupal.behaviors.autosaveForm = {

This is not the autosaveForm module ;-)

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2911446-6.patch3.19 KB
new interdiff-4-6.txt1.32 KB

Fixed the "typo" and hide the bundles if all is selected.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
StatusFileSize
new 2911446-interdiff-6-7.txt1.86 KB
new 2911446-7.patch3.35 KB

Again a small fix, when first time loading the settings form. In this case the bundles were not hidden.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
StatusFileSize
new 2911446-interdiff-7-8.txt2.11 KB
new 2911446-8.patch4.42 KB

Fixed options are not reseted between entities on settings form. This resulted in the last entity having all bundles of all entities ;)

Also fixed missing entities which doesn't have a bundle. You still can select all now in this case.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Needs review » Needs work

The patch looks much better now. Thank you, @kfritsche. I just have a couple of remarks.

  1. +++ b/js/content_lock_settings.js
@@ -0,0 +1,48 @@
+            .attr('disabled', true)
+            .prop('checked', false);
...
+          $checkbox.attr('disabled', false);

    According to the jQuery documentation disabled and checked are properties and should therefore be modified through the jQuery method "prop". Let's also add the drupal class "is-disabled" when disabling an element and also remove it when activating an element. 

  2. +++ b/src/Form/ContentLockSettingsForm.php
@@ -91,27 +91,33 @@ class ContentLockSettingsForm extends ConfigFormBase {
+        if ($definition->getBundleEntityType()) {
+          $bundles = $this->entityTypeManager

    Adding this condition leaves only the "*" option in case of entities not supporting bundles, but it might happen that it is planned that entity supports bundles at some point and the user wants to activate the content lock only for the default entity bundle, but having only the "*" option she could not limit the lock only on the default bundle. Does this use case make sense? 

  3. +++ b/src/Form/ContentLockSettingsForm.php
@@ -91,27 +91,33 @@ class ContentLockSettingsForm extends ConfigFormBase {
+          '#description' => $this->t('Select the bundles on which enable content lock'),

    "...on which TO enable..."

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2911446-10.patch4.52 KB
new 2911446-interdiff-8-10.txt1.49 KB

Thanks for the review, fixed 1+2

Adding this condition leaves only the "*" option in case of entities not supporting bundles, but it might happen that it is planned that entity supports bundles at some point and the user wants to activate the content lock only for the default entity bundle, but having only the "*" option she could not limit the lock only on the default bundle. Does this use case make sense?

I think exactly for this case its good that all bundles are locked. As the user always expected this behavior and when adding new bundle she might forget about content_lock setting, resulting in content not locked anymore, which worked before. In my mind this is correct. Most use-cases for content lock in my mind are anyways entity based and not bundle based.
I would leave it like this.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Needs review » Reviewed & tested by the community

Most use-cases for content lock in my mind are anyways entity based and not bundle based.

Well, I guess this makes sense :).

All the remarks of @chr.fritsch and me have been fixed and the change is pretty straightforward, therefore I think it should be safe to get this in. Thank you!

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

#2911532: Lock on entity translation level instead on entity level landed, so this needs a reroll.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2911446-13.patch5.39 KB

Re-roll party :)

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

You introduced all entity types to the settings page. Currently content_lock doesn't work with entity types without bundles. So we have to remove them.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented

@chr.fritsch it should work now with all entity types, as for entities without bundles you have the all selection now.

This makes it usable for entity_types without bundles. Also the used function ->bundle() always returns something. For entity_types without bundles this is the entity_type.

This was true for the case before, but with this patch is possible now.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

I activated it for the user entity type and went to user edit page. Then i got a 500.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2911446-interdiff-13-17.txt1.91 KB
new 2911446-17.patch6.84 KB

Bug confirmed. Thanks for testing and finding it.

Its now fixed.

In BreakLockRoutes I still checked for $definition->getBundleEntityType(), this is removed now and tested with the use case above.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs review » Needs work

Ok, the bug from #16 is now fixed. The bundle hiding on the settings page still doesn't work if you click 'All bundles'.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented

For all entity_types or only for entity_types without bundles?

Only way I can reproduce it, after a drush cr and firefox browser cache clean, if I click on it before the JS is loaded. Just because my system is to slow after I cleaned all caches. On second request everything loads to fast to reproduce it.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

I was first testing this in Thunder. In a clean Drupal installation it works. So seems that we broke something in Thunder.

  • chr.fritsch committed fb6b620 on 8.x-1.x authored by kfritsche 
    Issue #2911446 by kfritsche, hchonov: Add an option to the configuration...
chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Thank you, guys :-)

Status: Fixed » Closed (fixed)

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