Problem/Motivation

A relationship to the content_moderation_state entity was previously the only way to display and filter the moderation state for moderated entities. Now, views support has been built into the computed moderation_state field directly.

Reasons that deprecating this feature is a good idea:

  • The field should be the only way that developers and site builders need to reason about the storage of the moderation state, the entity in the background is marked @internal and there is even discussions about removing it entirely.
  • There are numerous bugs with the relationship: it creates weird text based exposed filters, it doesn't work with multiple languages, it doesn't add config dependencies etc.
  • Supporting it twice (via the field and via the relationship) includes a significant amount of additional code @see patch #6 and takes up resources in the issue queue.

Proposed resolution

Since relationships are more a feature of the Drupal "product" than an API, that is, it would be perfectly reasonable for someone to install Drupal, configure the relationship on a view without writing any code it has to be treated differently to most API deprecations. To solve this, there are two warnings in the UI which will tell users they are either about to use a deprecated feature or have already used it:

The change record will have detailed steps required to configure the supported field based views integration and steps on how to remove the relationship.

For folks who have done something heavily customised or extended the internal entity type in some way, a link to a contributed project will be provided which adds the relationships back.

The net result: less code to support and less confused users in the queue who don't understand why this relationship is so broken.

Remaining tasks

Agree on the approach and commit.

User interface changes

Additional UI elements pictured above.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
2.5 KB

I think this will be as easy as removing the 'relationship' part of the views data. We will likely need to re-think the views tests as well.

Sam152’s picture

Status: Needs review » Needs work

NW behind the blockers.

Sam152’s picture

Issue tags: +Workflow Initiative
timmillwood’s picture

Title: Remove the relationship from moderated content to the "content_moderation_state" entity » Remove the Views relationship from moderated content to the "content_moderation_state" entity

Adding "Views" to the issue title, initially I thought this was about removing the content_moderation_state entity (which might also be a good thing).

Sam152’s picture

Fixing up the tests for this. I don't think we need to replace the instances of the test with the concepts that supersede them in the blockers, as they would have gotten their own tests when they were added.

amateescu’s picture

This makes perfect sense! The issues linked in the issue summary will provide a much better site builder experience than joining an internal entity type and we won't expose implementation details to our users anymore :)

dawehner’s picture

Relationships are a fist class system in views. It is needed for stuff like the author of a node, hiding that is not necessarily the best idea.

I do agree that having filters/fields is a good idea, but is that a reason to drop support for the relationships in total? This also causes issues for everyone running existing sites and maybe have these relationships configured.

To be honest I get this as a task, but I don't see where the bug is exactly.

Sam152’s picture

Category: Bug report » Task

Wrong category, oops.

As someone with lots of these joins in production right now, I would be very happy to see this removed. The experience of adding the relationship for filtering and to display states is terrible. The filter shows up as a textfield and states show machine names, not labels. It's also really easy to screw up the join depending on if you want to show the state of the latest revision or the default revision.

Overall I think it's generally pretty broken and will be a source of confusion for users. If it's adding no value, based on the blockers being resolved, why not remove it?

Sam152’s picture

Title: Remove the Views relationship from moderated content to the "content_moderation_state" entity » [PP-2] Remove the Views relationship from moderated content to the "content_moderation_state" entity
Status: Needs review » Postponed

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

Sam152’s picture

Title: [PP-2] Remove the Views relationship from moderated content to the "content_moderation_state" entity » [PP-1] Remove the Views relationship from moderated content to the "content_moderation_state" entity
Sam152’s picture

With both blocking issues RTBC, I think it's worth considering this again. It's a touch point with the @internal entity that could very easily block us from being able to remove that in the future.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

Sam152’s picture

Title: [PP-1] Remove the Views relationship from moderated content to the "content_moderation_state" entity » Remove the Views relationship from moderated content to the "content_moderation_state" entity

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.

Sam152’s picture

Title: Remove the Views relationship from moderated content to the "content_moderation_state" entity » Deprecate the Views relationship from moderated content to the "content_moderation_state" entity
Status: Postponed » Active

This came up in a discussion in #2944789: Displaying moderation state on translated contents in admin content view. If it's now too late to remove this relationship, I wonder if we could trigger a deprecation warning about it instead?

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sam152 credited Berdir.

Sam152’s picture

@Berdir pointed out in slack that a hook_requirements warning, indicating that a user is using a feature of a module that is deprecated and slated to be removed might be a good way of signalling these kind of feature changes.

I wonder if that could work here? An error on the status page:

You have configured a view that creates a relationship to the "Content moderation state" entity type. This relationship will be removed in Drupal 9. You may either remove this relationship and instead use the "moderation state" field on the moderated entity type or install the "Content moderation entity relationship" contributed module.

I think something like that could work, but I think input from a committer or release manager would be valuable before going too far down this path.

catch’s picture

Is it impossible to write an upgrade path for this? (look for views with the relationship, remove it, add the moderation state field instead?)

Sam152’s picture

I think an upgrade path would come with way too many assumptions. I think it would be difficult to enumerate the scenarios users can actually get into by using the relationship. For example, the moderation_state field doesn't actually have a custom views field plugin associated with it, so it ends up being a textfield input. I think there is a high chance that folks who have added the relationship may have wrapped custom code around this. There is also a chance that additional relationships exist beyond the join to the content_moderation_state entity, you could add an additional relationship on the uid field.

To support this I think we should support folks who have used this and wrapped custom code around it in contrib, but remove it from core. It's easy to get into a situation where adding the relationship seems like the correct approach from the UI, but you're greeted with a fairly bad and broken experience beyond that.

Sam152’s picture

Status: Active » Needs review
FileSize
5.64 KB

Patch that uses the hook_requirements approach of deprecating this.

Sam152’s picture

Fleshed out issue summary.

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Fixing test groups.

alexpott’s picture

+++ b/core/modules/content_moderation/content_moderation.install
@@ -17,6 +17,22 @@ function content_moderation_requirements($phase) {
+            'severity' => REQUIREMENT_ERROR,

Should this be a warning or an error? If the view is working for the site then there's no error but we're advising them at some point they need to take steps.

Also I think we need to actually do proper plugin deprecations in the views so we can detect that we're not using the deprecated code path in core. Which I'm sure we still are. So we need to do something like the patch attached. But I think we should add generic view deprecated plugin handling in a another (blocking) issue.

Status: Needs review » Needs work

The last submitted patch, 28: 2894479-28.patch, failed testing. View results

Sam152’s picture

Also I think we need to actually do proper plugin deprecations in the views so we can detect that we're not using the deprecated code path in core. Which I'm sure we still are.

Are we? I think the field patches were committed before we shipped the moderated content view.

Re: deprecations, it might be tricky to do generic views plugin deprecation, since I don't think we can add the trigger_error in the constructor, since plugin manager may create instances of plugins for things like displaying them in a UI, but they may not actually be configured into a config object.

Sam152’s picture

Sam152’s picture

Status: Needs work » Needs review
FileSize
7.51 KB
13.25 KB

This puts the code deprecation into the relationship plugin base in a method where we would definitely consider the plugin to be in-use if it was called. I think we'd be stuck doing this on a case by case basis for different views plugins if this came up in the future.

The deprecation fails showed that we were using the relationship in ModerationStateAccessTest, which was easily replaced with the field version instead, which was kind of neat. The other fix was marking ViewsDataIntegrationTest as @legacy, since it's the test that is testing the relationships themselves.

Status: Needs review » Needs work

The last submitted patch, 32: 2894479-32.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
623 bytes
12.65 KB

Oops, the trigger_error in the constructor snuck back into the patch.

larowlan’s picture

+++ b/core/modules/content_moderation/content_moderation.install
@@ -17,6 +17,22 @@ function content_moderation_requirements($phase) {
+  if ($phase === 'runtime') {
+    $config = \Drupal::configFactory();
+    foreach ($config->listAll('views.view.') as $view_id) {
+      foreach ($config->get($view_id)->get('display') as $display) {

+++ b/core/modules/content_moderation/src/ViewsData.php
@@ -77,6 +78,7 @@ public function getViewsData() {
+          'deprecated' => 'Moderation state relationships are deprecated in drupal:8.8.0 and is removed in drupal:9.0.0. See https://www.drupal.org/node/3061099',

@@ -102,6 +105,7 @@ public function getViewsData() {
+          'deprecated' => 'Moderation state relationships are deprecated in drupal:8.8.0 and is removed in drupal:9.0.0. See https://www.drupal.org/node/3061099',

Given we declare the fact that a relationship is deprecated in the views data, could we not have the views module present this warning for us?

e.g. lets say another module needs to do similar - it would now need its own hook_requirements to do essentially the same thing.

Could it be generalized in views and therefore extended to other views features like fields, filters and so on.

Sam152’s picture

That does sound like a really good idea if there were more instances of this popping up.

To my knowledge there isn't another chunk of views data that is on the chopping block, so given the extra code and complexity of making it generic it might be worth delegating that to a follow up, contingent on it becoming a more widespread problem.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

++1 to #36 - let's open a follow-up for this.

+++ b/core/modules/content_moderation/content_moderation.install
@@ -17,6 +17,22 @@ function content_moderation_requirements($phase) {
+          $requirements['deprecated_views_relationship'] = [
+            'title' => t('Content Moderation State views relationship'),
+            'description' => t('This installation contains one or more views which is using a relationship to the Content Moderation State entity. This relationship is deprecated and will be removed before 9.0.0. See <a target="_blank" href=":url">this change record</a> for information on removing this relationship or alternative solutions.', [':url' => 'https://www.drupal.org/node/3061099']),
+            'severity' => REQUIREMENT_ERROR,
+          ];

I think that not listing the view's here with links to edit them is a UX issue. Otherwise how is someone going to know what to fix.

Sam152’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
3.53 KB
96.02 KB
13.75 KB

Thanks for reviewing! :)

Adding a list of links to views that require updating, updating the image in the issue summary with the message in the latest patch, adding a follow-up to reference if other modules end up needing this: #3087292: Allow views to warn users when fragments of views configuration are designated to be deprecated..

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
13.46 KB
770 bytes

Reviewed the patch and I think it looks great! There was just one minor problem: #3037136: Make Workspaces and Content Moderation work together was committed, so content_moderation_requirements() needed a small reroll to remove the incompatibility requirement.

@alexpott's review from #37 was also addressed and the followup created, so RTBC :)

alexpott’s picture

Updating credit

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 1d386c5ddb to 9.0.x and 6083a092c4 to 8.9.x and 2b43c2594f to 8.8.x. Thanks!

  • alexpott committed 1d386c5 on 9.0.x
    Issue #2894479 by Sam152, alexpott, amateescu, Berdir, larowlan:...

  • alexpott committed 6083a09 on 8.9.x
    Issue #2894479 by Sam152, alexpott, amateescu, Berdir, larowlan:...

  • alexpott committed 2b43c25 on 8.8.x
    Issue #2894479 by Sam152, alexpott, amateescu, Berdir, larowlan:...

Status: Fixed » Closed (fixed)

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

matthieuscarset’s picture

Warning:

Now that the relationship has been removed, all Views using moderation_state as Contextual filter will be broken.

We need to implement a contextual filter for Moderation State.

quietone’s picture

Publish the change record