Problem/Motivation

Support other than nodes

Proposed resolution

Use a generic form alter
Remove node-specific type hints
Use config schema alter to add the third party stuff to all config entities

Remaining tasks

Update the Param Converter to not be node specific

Update ModerationInformation::isModeratableBundle() (which has a NodeType::load() call in it that needs to go away, but I'm still lost in the labyrinth of entities used to define entities to do that easily myself.

Do more testing and ensure it actually still works.

Expand EditTab to apply to all relevant entities, not just nodes.

Expand ModerationStateWidget to apply to all relevant entities, not just nodes.

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

larowlan created an issue. See original summary.

Crell’s picture

Assigned: Unassigned » Crell

I am working on this.

dawehner’s picture

So i'm wondering whether the right approach would be to have a UI kinda like content/config translation has which is a central page to configure what is moderatable.

Another approach could be to add the moderation always by adding a field via the field UI, but I guess this could add too much complexity, though also improve things fundamentally.

Crell’s picture

Status: Active » Needs work
FileSize
21.67 KB

I've attached a patch of my current status. This is still a WIP, but it gets a sense for how I'm approaching it. The current problem I have is that I cannot figure out how to get the Local Task Deriver to wire up the workflow form properly. I suspect the issue is that I just don't grok how the base route thing is supposed to work. Suggestions welcome.

I will continue work on this on Monday, so just let me know what I'm doing wrong and I'll try to finish it up then. :-)

Side note: I definitely think there's room to factor much of what I'm doing out to a utility in entity module. Adding a new config form to an entity type is a common-enough task (I'd think) that most of this boilerplate can and should be generalized. Let's get it working here first, though, and then I can spin it out.

larowlan’s picture

FileSize
3.01 KB
21.71 KB

Pushed this, working for nodes at least

dawehner’s picture

  1. +++ b/src/Plugin/Derivative/DynamicLocalTasks.php
    @@ -0,0 +1,96 @@
    +          if ($entity_types[$type->get('bundle_of')]->isRevisionable()) {
    +            yield $type_name => $type;
    +          }
    

    Does a yield really makes sense if you don't do any costly operation in the meantime?

  2. +++ b/src/Routing/WorkflowRouteProvider.php
    @@ -0,0 +1,65 @@
    +          '_title' => 'Workflow', // @todo Translate this.
    

    _title is translated automatically

Crell’s picture

The generator isn't being done for performance but code cleanliness. I originally was using a set of filter iterators, one for each conditional, but decided that the 3 nested if statements and a generator was cleaner and about half as much code. :-) It still separates the "which do I operate on" and "what is the operation" steps into 2 separate methods, which was the goal.

larowlan: you'll need to explain to me on Monday how that thing works. :-)

jibran’s picture

@Crell have you added @larowlan to your mentors list yet? :P

Crell’s picture

I have not, but I suspect by the time this issue is committed I will have added him about 3 times. :-)

dawehner’s picture

FileSize
21.81 KB

The generator isn't being done for performance but code cleanliness. I originally was using a set of filter iterators, one for each conditional, but decided that the 3 nested if statements and a generator was cleaner and about half as much code. :-) It still separates the "which do I operate on" and "what is the operation" steps into 2 separate methods, which was the goal.

I love them and I used it already in many D8 custom modules.
While I agree with in general for large datastes its a bigger pain if you are actually in a debugger, because you jump back and forth all the time,
so what about something like this?

dawehner’s picture

FileSize
2.87 KB

Missing interdiff

Crell’s picture

You don't want me to have any fun with new language features, do you... :-P Applied #11 and removed the other generator, too. Now following up on larwolan's commit to figure out if his works and why.

dawehner’s picture

:( Did not wanted to disappoint you, generators are the best when you do something like HTTP requests.

Crell’s picture

Issue summary: View changes

Latest snapshot of work here. This code is also in the entity branch.

There is still work to be done here, but for time reasons I'm going to ask for backup. Don't wait for me at this point. :-) I'll be back Monday.

What I've learned is that the forms for different entity types are still complete one-offs, so globally editing those is not going to fly. I've hard-coded Node and BlockContent for now, as that's what core provides, but I do not like the approach.

I also fixed #2630594: Forward revisions after initial publication not active along the way, since it got in the way of manual testing. (I added automated tests for that, too, with dawehner's help.) That also necessitated forcing revisions to always be on for a moderatable bundle, which larowlan agreed in IRC was a Good Thing(tm).

Updated the issue summary with the remaining tasks.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs work » Needs review
FileSize
75.8 KB

The basic test coverage works now again, the todos are addressed.

/me would vote for getting it in rather sooner than later in order to not just wait.

larowlan’s picture

Agree

dawehner’s picture

I won't be here tonight, so feel free to take it over, improve bits and maybe commit it one day?

jibran’s picture

  1. +++ b/moderation_state.services.yml
    @@ -1,6 +1,17 @@
    +    tags:
    ...
    +    tags:
    

    We can remove these.

  2. +++ b/src/EntityOperations.php
    @@ -0,0 +1,57 @@
    +class EntityOperations {
    
    +++ b/src/EntityTypeInfo.php
    @@ -0,0 +1,205 @@
    +class EntityTypeInfo {
    
    +++ b/src/ModerationInformation.php
    @@ -0,0 +1,170 @@
    +class ModerationInformation {
    

    Let's add an interface.

  3. +++ b/src/EntityTypeInfo.php
    @@ -0,0 +1,205 @@
    +   * @see hook_entity_bundle_field_info_alter();
    ...
    +   * @see hook_form_alter()
    ...
    +   * @see hook_form_alter()
    

    @see should be on the last line.

  4. +++ b/src/EntityTypeInfo.php
    @@ -0,0 +1,205 @@
    +   * @param array $fields
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    +   * @param string $bundle
    ...
    +   * @param $form
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   * @param $form_id
    ...
    +   * @param $form
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   * @param $form_id
    

    @param desc is missing.

  5. +++ b/src/EntityTypeInfo.php
    @@ -0,0 +1,205 @@
    +    return;
    

    Not required.

  6. +++ b/src/ModerationInformation.php
    @@ -0,0 +1,170 @@
    +      && $type->get('bundle_of')
    +      && $entity_types[$type->get('bundle_of')]->isRevisionable();
    

    Let's do this instead ($bundle_of = $type->get('bundle_of'))
    $entity_types[$bundle_of]->isRevisionable()

  7. +++ b/src/ModerationInformation.php
    @@ -0,0 +1,170 @@
    +   * Determines if a config entity is a bundle for entities that may be moderated.
    

    More then 80 chars.

jibran’s picture

FileSize
20.13 KB
85.51 KB

Fix #23

jibran’s picture

Needs to merge 8.x-1.x

+++ b/src/EntityTypeInfo.php
@@ -0,0 +1,205 @@
+    if ($entity instanceof Node) {
...
+    else if ($entity instanceof BlockContent) {

What about media entities?

Crell’s picture

+++ b/src/EntityTypeInfo.php
@@ -25,11 +25,14 @@ use Drupal\node\Entity\NodeType;
+class EntityTypeInfo implements EntityTypeInfoInterface{

Nit: Missing space.

That said, I'm not sure the interface is necessary for a service that is, really, just a place to stick alter hooks. I'm not going to block it but it feels excessive, even to me. :-)

Please note: We should merge this branch, not commit the patch! (So no one commit a patch from the issue, please.)

jibran: In places where we do that, it's because there is not, as far as I am aware, any commonality between those entities we can rely on for the code in question. The only core entities that this module applies to right now are Nodes and Block Content, so that's what it's hard-coded to. I hate switch-statement logic, but I don't know of a better alternative for now. :-( Subsequent work can/should probably come up with a better solution, likely fixing core to provide better automation we can latch on to or at least allowing for another tagged service behind this one to handle such branching and let contrib entities hook into it themselves.

Fortunately AFAIK this is only an issue for admin forms right now, so the issue is primarily cosmetic, not functional. We definitely want to allow Media, Commerce Products, etc. to be moderatable.

dawehner’s picture

That said, I'm not sure the interface is necessary for a service that is, really, just a place to stick alter hooks. I'm not going to block it but it feels excessive, even to me. :-)

I can't agree more. There is a general problem in the community that interfaces are not understood. They are used simply to put a couple of public methods on there and this is all, but well, interfaces are about making it clear where your extensions and communications points are.

jibran’s picture

but it feels excessive, even to me. :-)

Oh my what have I done :P

Ok I got the point. Interface for EntityOperations and EntityTypeInfo don't make sense but I think creating an interface for ModerationInformation makes sense because we are injecting it in two service. Thoughts?

dawehner’s picture

+1 for those.

jibran’s picture

Ok I pushed b9154f6.

but I don't know of a better alternative for now.

I don't think implementing a plugin manager for this would be a crazy idea. ER has selection plugins for this kind of dirty work.

dawehner’s picture

Thank you @jibran!

Crell’s picture

A plugin manager would be overkill here, as these are all entirely static, code-time services. The DI container has all the plugin-ish functionality we actually need.

Crell’s picture

FileSize
82.24 KB

I've cleaned up some remaining bugs with the entity branch related to BlockContent being not-Nodes. *sigh* I also rebased against 8.x-1.x and pushed a new branch called all-entities. That's where work should continue.

There appears to be a test failure with the validator now, introduced after the rebase. I don't have time to investigate that now. If someone else does, go for it. :-)

Once all tests pass, I say we merge this branch and continue in #2634238: Add moderation handlers to hold entity-type-specific logic. Since this branch is already rewriting 2/3 of the module, we may as well make it official.

Attached diff is of the all-entities branch against 8.x-1.x. Note that I removed the unnecessary trait in favor of just moving that method to ModerationInformation, and cleaned up some other ugly code along the way.

jibran’s picture

Nice work just minor review and some suggestions.

  1. +++ b/src/Form/EntityModerationForm.php
    @@ -0,0 +1,159 @@
    +   * @todo I don't know why this needs to be separate from the form() method.
    

    @larowlan can you have a look at it?

  2. +++ b/src/Form/EntityModerationForm.php
    @@ -0,0 +1,159 @@
    +   * @inheritDoc
    

    oops

  3. +++ b/src/ModerationInformation.php
    @@ -0,0 +1,167 @@
    +  public function __construct(EntityTypeManagerInterface $entity_type_manager, AccountInterface $current_user) {
    

    Doc block missing.

  4. +++ b/src/ModerationInformation.php
    @@ -0,0 +1,167 @@
    +   * @todo There may be a more perfomant way of doing this that doesn't
    +   * require loading the full entity revision.
    

    Maybe ask @berdir?

  5. +++ b/src/Routing/ModerationRouteProvider.php
    @@ -0,0 +1,65 @@
    +   * @inheritDoc
    

    oops

  6. +++ b/tests/src/Kernel/EntityOperationsTest.php
    @@ -0,0 +1,153 @@
    +  public static $modules = ['moderation_state', 'node', 'views', 'options', 'user', 'system'];
    

    Maybe add a test for block content and block content type as well. Totally go into follow up.

dawehner’s picture

Status: Needs review » Fixed

* Fixed the actionable items in the review of @jibran
* Fixed the test failure as described by crell
* Fixed the test failure, please review the fixes I made there.

Merged the branch into 8.x-1.x

Great teamwork!
See you in more issues!

Status: Fixed » Closed (fixed)

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