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

NW for deprecation message

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

The last submitted patch, 40: 3167198-40.patch, failed testing. View results →

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 🐛 [PP-1] FilterFormat should add the roles as config dependencies Postponed .

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 📌 [PP-2] Make FilterFormat config entities fully validatable Postponed .

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 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 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! 😊

dimitriskr → changed the visibility of the branch 11.x to hidden.

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?

@Wim Leers summoned me to take another look at this. Really looks great, have no objections. I did review all the changes since my last patch and they are all fine, but since I did author a good chunk of what is still in the current patch, not going to RTBC as people do sometimes get very sensitive with that. Total endorsement of this landing, though, would really love to see this in for all kinds of reasons! Thanks everyone for pushing this along and AFAICT very close to the finish line.

#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! 😄

Awesome, thanks!

Yeah, I totally agree, just want to avoid getting into arguments with people...

But then RTBC++, I guess ;-)

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 📌 Deprecate the 'roles' property of text formats Needs review Do we still need this?

I've pushed some code changes.
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).