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.
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-40.txt | 770 bytes | amateescu |
#40 | 2894479-40.patch | 13.46 KB | amateescu |
#38 | 2894479-38.patch | 13.75 KB | Sam152 |
#38 | Screen Shot 2019-10-11 at 10.00.44 am.png | 96.02 KB | Sam152 |
#38 | interdiff.txt | 3.53 KB | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 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.
Comment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedNW behind the blockers.
Comment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #5
timmillwoodAdding "Views" to the issue title, initially I thought this was about removing the content_moderation_state entity (which might also be a good thing).
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing 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.
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis 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 :)
Comment #8
dawehnerRelationships 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.
Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWrong 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?
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWith 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.
Comment #14
xjmComment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #18
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis 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?
Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@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:
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.
Comment #22
catchIs it impossible to write an upgrade path for this? (look for views with the relationship, remove it, add the moderation state field instead?)
Comment #23
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 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 thecontent_moderation_state
entity, you could add an additional relationship on theuid
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.
Comment #24
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedPatch that uses the
hook_requirements
approach of deprecating this.Comment #25
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFleshed out issue summary.
Comment #26
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #27
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing test groups.
Comment #28
alexpottShould 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.
Comment #30
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAre 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.Comment #31
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis 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 markingViewsDataIntegrationTest
as@legacy
, since it's the test that is testing the relationships themselves.Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOops, the
trigger_error
in the constructor snuck back into the patch.Comment #35
larowlanGiven 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.
Comment #36
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThat 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.
Comment #37
alexpott++1 to #36 - let's open a follow-up for this.
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.
Comment #38
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks 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..
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedReviewed 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 :)
Comment #41
alexpottUpdating credit
Comment #42
alexpottCommitted and pushed 1d386c5ddb to 9.0.x and 6083a092c4 to 8.9.x and 2b43c2594f to 8.8.x. Thanks!
Comment #47
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedWarning:
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.
Comment #48
quietone CreditAttribution: quietone at PreviousNext commentedPublish the change record