Deprecate the 'roles' property of text formats and remove it from the UI

1. Nice work - great idea to get rid of this

2. +++ b/core/modules/filter/src/FilterFormatFormBase.php
   @@ -168,6 +169,15 @@ public function exists($format_id) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function copyFormValuesToEntity(EntityInterface $entity, array $form, FormStateInterface $form_state) {
   +    // Avoid setting the roles as a property of the text format.
   +    $entity->set('name', $form_state->getValue('name'));
   +    $entity->set('format', $form_state->getValue('format'));
   +  }
   
   @@ -202,14 +212,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
        foreach ($form_state->getValues() as $key => $value) {
   -      if ($key != 'filters') {
   -        $format->set($key, $value);
   -      }
   -      else {
   +      if ($key === 'filters') {
            foreach ($value as $instance_id => $config) {
              $format->setFilterConfig($instance_id, $config);
            }
          }
   +      elseif ($key !== 'roles') {
   +        $format->set($key, $value);
   +      }
        }
   

   There's an interesting tension here. in HEAD \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity loops around $form_state->getValues() setting all the values on the entity apart from the filters - because thats the plugin collect key.

   With this change we're hardcoding the name and format values in copyFormValuesToEntity but we're still looping around in submitForm.

3. +++ b/core/modules/filter/tests/filter_test/config/install/filter.format.filter_test.yml
   @@ -1,9 +1,6 @@
   -roles:
   -  - anonymous
   -  - authenticated
   

   If we want to avoid causing modules any problems we could implement hook_ENTITY_TYPE_insert in the filter test module. That way any contrib relying on this test config doesn't have to change. As the tests changed here should the update is simple and will work on Drupal 9.0 too. However adding the hook might be worth it given some modules do install the filter_test module in their tests - see http://codcontrib.hank.vps-private.net/search?text=%27filter_test%27&fil...

4. +++ b/core/modules/filter/tests/src/Kernel/FilterLegacyTest.php
   @@ -0,0 +1,74 @@
   +    // Verify that the loaded format does not contain any roles.
   +    $this->assertNull($format->get('roles'));
   

   This is what causes the deprecation to be triggered. So the test is testing itself. I think we need to trigger the deprecation in the postSave()... specifically in

       if (!$update && !$this->isSyncing()) {
         // Default configuration of modules and installation profiles is allowed
         // to specify a list of user roles to grant access to for the new format;
         // apply the defined user role permissions when a new format is inserted
         // and has a non-empty $roles property.
         // Note: user_role_change_permissions() triggers a call chain back into
         // \Drupal\filter\FilterPermissions::permissions() and lastly
         // filter_formats(), so its cache must be reset upfront.
         if (($roles = $this->get('roles')) && $permission = $this->getPermissionName()) {
           foreach (user_roles() as $rid => $name) {
             $enabled = in_array($rid, $roles, TRUE);
             user_role_change_permissions($rid, [$permission => $enabled]);
           }
         }
       }
   

   Ah that is going to trigger the deprecation. Because of $this->get('roles'). So I guess this is okay but also I think we should not make calls in the test that trigger the deprecation so that we can test that regular runtime usage does indeed trigger it as expected.

5. +++ b/core/modules/filter/tests/src/Kernel/FilterLegacyTest.php
   @@ -0,0 +1,74 @@
   +    // Verify role permissions declared in default config.
   +    $format = FilterFormat::load('filter_test_legacy');
   +    $this->assertEquals([
   +      RoleInterface::ANONYMOUS_ID,
   +      RoleInterface::AUTHENTICATED_ID,
   +    ], array_keys(filter_get_roles_by_format($format)));
   

   I think we should use a filter format without the roles key for this test now - so that we can be sure that setting it what triggers the deprecation.

6. +++ b/core/modules/filter/tests/src/Kernel/FilterLegacyTest.php
   @@ -0,0 +1,74 @@
   +    $format->save();
   

   I think we should assert that the roles key is not present when toArray() is called. After making this save.

7. +++ b/core/profiles/standard/config/install/filter.format.full_html.yml
   @@ -6,8 +6,6 @@ dependencies:
   -roles:
   -  - administrator
   

   This works because the administator role is given every permission. Nice.

This is looking really nice.

1. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -48,7 +48,6 @@
   - *     "roles",
   

   Also we need to use the new config schema deprecation stuff to deprecate the the roles schema.

2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -439,4 +427,30 @@ protected function calculatePluginDependencies(PluginInspectionInterface $instan
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function get($property_name) {
   +    $value = parent::get($property_name);
   +    // Trigger a deprecation for the deprecated 'roles' property if any roles
   +    // are set.
   +    if (($property_name === 'roles') && $value) {
   +      $class_name = static::class;
   +      @trigger_error("The property $property_name is deprecated in $class_name and will be removed before Drupal 10.0.0, see https://www.drupal.org/node/3168851.", E_USER_DEPRECATED);
   +    }
   +    return $value;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function set($property_name, $value) {
   +    // Trigger a deprecation when setting deprecated 'roles' property.
   +    if ($property_name === 'roles') {
   +      $class_name = static::class;
   +      @trigger_error("The property $property_name is deprecated in $class_name and will be removed before Drupal 10.0.0, see https://www.drupal.org/node/3168851.", E_USER_DEPRECATED);
   +    }
   +    parent::set($property_name, $value);
   +  }
   

   Done some thinking about this added code. I don't think it is necessary. I think we can add the deprecation to \Drupal\filter\Entity\FilterFormat::postSave() as this is the only place where the roles info is used and can possibly exist because the way it is working is HEAD is that the roles property only exists while we're creating the filter config entity. Due to the head implementation of toArray() it is always removed.

3. \Drupal\filter\Entity\FilterFormat::$roles needs to be deprecated to I think.

See drupal.org/node/3129881 for how to deprecation bits of schema.

Marking #1901636: Importing a disabled format that is assigned to a role causes a fatal as duplicate, and bumping this to major in its stead, since this issue might fix several other ones.

Looks good in general, just a few points:

1. +++ b/core/modules/filter/config/schema/filter.schema.yml
   @@ -27,6 +27,7 @@ filter.format.*:
   +      deprecated: "The 'roles' property of text formats is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3168851."
   

   The version needs bumping up.

2. +++ b/core/modules/filter/src/FilterFormatFormBase.php
   @@ -168,6 +169,19 @@ public function exists($format_id) {
   +    // Avoid setting the roles as a property of the text format.
   +    $entity->set('name', $form_state->getValue('name'));
   +    $entity->set('format', $form_state->getValue('format'));
   +
   +    foreach ($form_state->getValue('filters') as $instance_id => $config) {
   +      $entity->setFilterConfig($instance_id, $config);
   +    }
   

   I don't like how this is having to do all the properties that are NOT 'roles'.

   Could we use $form_state->unsetValue() before the parent class gets hold of it?

3. +++ b/core/modules/filter/src/FilterFormatFormBase.php
   @@ -196,31 +210,17 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    return $status;
   

   This looks like an unrelated fix.

Postponed #2569741: [PP-1] FilterFormat should add the roles as config dependencies on this.

Rerolled. #2852557: Config export key order is not predictable, use config schema to order keys for maps was the main conflict.

#30:

1. ✅ Fixed.
2. ✅ Done.
3. 👎 I think it's an essential fix because of the change from ::submitForm() (which used to update permissions and the FilterFormat config entity) to copyFormValuesToEntity() (to only update the FilterFormat config entity), which then means @tstoeckler had no choice but to update the permissions in ::save().

NW for deprecation message

Re-rolled patch at #33 against 11.x-dev

needs to change deprecation message and not clear about upgrade path

Tests finally pass

NR for what's currently in the MR and NW for the upgrade path

Updated remaining tasks in IS. CR needs update for updating drupal versions in body

Nice work here, @dimitriskr! But rather than expecting deprecations in tests, we should be updating the tests to not create text formats with the roles property anymore.

The default config of install profiles has been updated already, we just need to do the same for config in tests 😊

This blocks #2569741: [PP-1] FilterFormat should add the roles as config dependencies.

Thanks for the review Wim! One question. In some previous issues regarding deprecation, it had to have one test that would test that the deprecation message would pop up.

I'll remove the deprecated config from the test configuration but shouldn't exist one test to test the deprecation message? (by creating a separate test module with the deprecated config for this exact purpose)

it had to have one test that would test that the deprecation message would pop up.

That's right.

I'll remove the deprecated config from the test configuration but shouldn't exist one test to test the deprecation message? (by creating a separate test module with the deprecated config for this exact purpose)

Yes, this is the way 😊👍

This blocks #3421946: [PP-2] Make FilterFormat config entities fully validatable.

Alright, I'll continue on this.

And what about the upgrade path? Don't we need to change the configuration of the existing installations?
I was thinking about a hook_post_update (since it's configuration) and load all filters and remove the roles property. Would this be enough?

If there is an example on testing the upgrade path, I could add this too.

I've removed the expectDeprecation calls in the tests, as also remove the roles property in the formats

I've kept it in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest since it loads config from a folder called ckeditor4_config and don't want to mess up anything there. If it's ok to remove it, let me know.

The deprecation message testing already exists in \Drupal\Tests\filter\Kernel\FilterLegacyTest

> And what about the upgrade path? Don't we need to change the configuration of the existing installations?

No, because as stored configuration, the property isn't there anyway, This only affects default config files, and you can't files in an update function.

> I've kept it in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest since it loads config from a folder called ckeditor4_config and don't want to mess up anything there. If it's ok to remove it, let me know.

I think it's fine to change that. the roles/permission are not actually tested by that test I think, they look like they were copy pasted from standard profile (an old, ckeditor4 version of them). You most likely don't even need to add any permissions, unlike some of the other tests I commented on.

The failures in the Functional Tests mean that the test user has no access in ckeditor5.upload_image so it cannot upload the image.

We have to set to the user the permission to use the text format, with maybe the following? Otherwise the tests will fail. (tested locally)

    $format_permission = FilterFormat::load('basic_html')->getPermissionName();
    user_role_grant_permissions(RoleInterface::AUTHENTICATED_ID, [$format_permission]);

Or, if we move the calls of $this->createBasicFormat(); to setUp() (since it's called in every test method + in inherited ImageUploadAccessTest), we can add the permission as parameter in drupalCreateUser()

As for the FunctionalJavascript fails, I have no idea what is going on.

@Berdir++ in #53 😊

#54: I'll debug this. Thanks for pushing it so far already! 🙏

As far as I'm concerned, this is now RTBC-worthy. Updated the change record.

Hoping @Berdir will RTBC 😇

Actually, I have two more questions:

1. The issue summary says we need an update path. But … do we? Whenever the "Roles" checkboxes for text formats are used in the UI, the granted permissions already are already updated on the Role entities. So AFAICT there's nothing to be done? → tagged Needs issue summary update for this
2. Deprecating the role property is one thing, but what about the UI part? What about functional tests that are relying on the Roles checkboxes? Keeping them around for Drupal 10.x makes sense (to avoid a BC break), but in Drupal 11, they should disappear? Seems like we need a follow-up issue for that? Or do we consider that a natural consequence of this disappearing as soon as Drupal 11 actually removes the roles property?

Finally, closed #2569741 in favor of this, see explanation at #2569741-57: [PP-1] FilterFormat should add the roles as config dependencies.

Thanks Wim for taking care of this, it was getting difficult for me

1. I put the upgrade path to the tasks, since I thought it was necessary. Since #53, it was made clear it's not necessary, so we can remove it

2. Is it possible to point a URL to /admin/people/permissions, in the filter module permissions so they can be set directly from there? Or just tell people that in Drupal 11, in order to set permissions for the text formats, you must go to the URL above, in the release notes. And add a note in the current field that in 11 this will change

#59:

1. 👍 Thanks for confirming! Updated issue summary.
2. We should not modify the UI with warnings about what will change — that'd certainly cause a usability regression.

   On second thought … I don't think we need special consideration for this — literally EVERYTHING ELSE in Drupal requires going to the "permission" UI. This is the only inconsistency left AFAIK.

That means this now is truly RTBC-worthy! 😊

Subsequently add a deprecation notice in FilterFormat::get() and FilterFormat::set() for the roles property.

This part is missing. But … I'd argue it's not necessary, since FilterFormat::postSave() will still trigger a deprecation notice. It might be a slightly nicer/clearer way for contributed/custom module maintainers to be notified though?

I think the UI question is entirely disconnected from this issue. It's only related because it requires special workarounds to not trigger the deprecation because config entity forms by default just throw everything the form defines directly onto the entity. We could in theory revert this change in D11, it would just set an undefined property on the object but won't do anything with it (like many, many other form elements)

I do agree that this is special, but who can access which text format is IMHO such a central question that I definitely see this being useful here. That said, we have a pattern now with a permissions local task on e.g. node types that we could also use for this, it would be a bit overkill as it's always going to be a single permission only though.

That's really helpful, @Berdir — and I'd forgotten about the "permissions local task" pattern! 👍

You've convinced me that this issue is indeed 100% independent of any UI impact. Which means it's definitely in an RTBC'able state 🥳

Applied the suggestions by @Wim Leers.

Will this need an upgrade path though to remove roles key from existing config?

#66: it's deprecated, not removed. See #53 for why that is not necessary.

#67: We're more pragmatic than that at this point 😄 It's been a long time since you touched this, plus I reviewed YOUR changes, so if you can review mine and @dimitriskr's , then … there's no reason you cannot RTBC this! As @alexpott likes to say (paraphrasing)
: "why would you not be allowed to RTBC it if you happen to have the expertise to thoroughly review this particular area?" 😊

… and in that same vein: I've re-reviewed everything and only made trivial tests-only changes to the CKEditor 5 tests, to ensure that @group legacy only appears in the sole legacy test.

Let's get this done! 😄

One small and one slightly larger question on the MR - would be good to document this in the issue summary and we probably need an explicit follow-up to track removal.

This is one where I think we should deprecate for removal in Drupal 11 since we're maintaining a non-trivial bc layer and at worst this will affect shipped config.

@catch See my comment on the MR — I'd be fine with removing this UI, I just figured we're trying to minimize change for site builders during the D10 cycle too. Happy to do so though — it'd mean a more consistent UX anyway, so I sort of see the appeal: consistent UX and less code complexity!

@catch in Drupal Slack:

I think we should and can get rid of the UI in a minor. Will need a change record/release note.

So: let's do that! (Separate change record for the UI change 🙏)

@dimitriskr: Are you interested in making that part happen? :D

Sure, I can give it a go!

Could you also update the remaining tasks in IS?

#62 Do we still need this?

I've pushed some code changes and asked some clarifications too.
There needs to be a change at \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5TestBase::createNewTextFormat in order to grant the required permission to the current user to allow using each text format. I guess that will make CKEditor5 tests pass and the whole pipeline go green

Tests now pass :)

Thanks! Will review 🤓

@Wim Leers just following up if you had a chance to review this one? Think it would be a good thing for 11.x

Posted a review.

FWIW, I still think that the UI change should be a separate issue even if it adds a bit of technical dept on the form.

I always thought this is pretty convenient, because setting correct permissions is pretty much mandatory for a new filter format. Without this, it will not show up for users and there is no indication or feedback that you need to go to a completely different section of the UI to then grant users the permission to actually use this format.

IMHO we need some sort of replacement for that (a message maybe, or the mentioned permissions local task, but unlike bundles, saving does not bring us to a page where we'd see that local task). I think this change needs a UX review (and that's exactly why it should IMHO be separate from the technical change).

Adding #3519300: Provide a way to deprecate form API array value keys as related, because I'm not sure whether the roles form API array key should to be deprecated before it's removed.

1. Shall we wait for #3519300: Provide a way to deprecate form API array value keys and then deprecate $form['roles'] field properly? I think that covers comments from berdir and catch in MR and here, if I'm not mistaken.
2. There are some open threads on the MR I cannot answer
3. Drupal\Tests\filter\Functional\FilterSecurity fails because it cannot disable the format, but the code tests it can edit the format.
entity.filter_format.disable route returns access denied.
entity.filter_format.edit_form is OK
I'm investigating
4. Drupal\Tests\filter\Functional\FilterHooks fails because it cannot select the filter on the node add form, below the editor