... and translated content cannot be moderated :-D

Scenario:

1. Install content translation module.
2. Add French language.
3. Enable content translation on articles.
4. Create an article.
5. Add a French translation.
6. When saving the translation:

Expected: the translation is created.
Actual: this fatal error shows up:

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: Invalid translation language (fr) specified. in Drupal\Core\Entity\ContentEntityBase->getTranslation() (line 748 of core/lib/Drupal/Core/Entity/ContentEntityBase.php).
Drupal\node\NodeViewBuilder::renderLinks('2', 'full', 'fr', )
call_user_func_array('Drupal\node\NodeViewBuilder::renderLinks', Array)
Drupal\Core\Render\Renderer->doRender(Array)
Drupal\Core\Render\Renderer->doRender(Array, )
Drupal\Core\Render\Renderer->render(Array)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1)
__TwigTemplate_99b41775e98c1b5ee4bdedc3000b7c98395ed541e98c03ab696a6fa0ba631941->doDisplay(Array, Array)
Twig_Template->displayWithErrorHandling(Array, Array)
Twig_Template->display(Array)
Twig_Template->render(Array)
twig_render_template('core/themes/bartik/templates/node.html.twig', Array)
Drupal\Core\Theme\ThemeManager->render('node', Array)
Drupal\Core\Render\Renderer->doRender(Array, )
Drupal\Core\Render\Renderer->render(Array, )
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
CommentFileSizeAuthor
#30 interdiff.txt5.46 KBjuampynr
#30 moderated_content-2654972-30.patch11.47 KBjuampynr
#22 interdiff.txt7.1 KBjuampynr
#22 moderated_content-2654972-22.patch14.95 KBjuampynr
#19 moderated_content-2654972-19.patch12.58 KBjuampynr
#19 interdiff.txt5.68 KBjuampynr
#16 moderated_content-2654972-16.patch10.52 KBjuampynr
#16 interdiff.txt10.76 KBjuampynr
#13 moderated_content-2654972-13.patch10.15 KBjuampynr
#12 moderated_content-2654972-12.patch4.45 KBjuampynr
#12 interdiff.txt3.25 KBjuampynr
#8 moderated_content-2654972-8.patch3.54 KBjuampynr
#2 moderated_content-2654972-2.patch2.9 KBjuampynr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juampynr created an issue. See original summary.

juampynr’s picture

Here is a test that demonstrates the issue. I will start debugging what is going on.

Status: Needs review » Needs work

The last submitted patch, 2: moderated_content-2654972-2.patch, failed testing.

The last submitted patch, 2: moderated_content-2654972-2.patch, failed testing.

The last submitted patch, 2: moderated_content-2654972-2.patch, failed testing.

Crell’s picture

Priority: Normal » Major
Crell’s picture

Priority: Major » Critical

Fatal error = critical. :-(

juampynr’s picture

This is where I am at: removing this line lets you to add a translation. However, tests fail with this change so I am going through them to understand the implications and adjust the patch further.

Status: Needs review » Needs work

The last submitted patch, 8: moderated_content-2654972-8.patch, failed testing.

The last submitted patch, 8: moderated_content-2654972-8.patch, failed testing.

The last submitted patch, 8: moderated_content-2654972-8.patch, failed testing.

juampynr’s picture

Making progress. It is taking me a long time to understand what happens in core and WBM when a new translation is created. I managed to fix this, but now I need to debug what happens when subsequent drafts are created. I will continue working on it and update this patch.

juampynr’s picture

I looked at how core does content translation and saw that an editor can publish node translations independently, which makes sense to me. Therefore, I adjusted this module so now translations can be moderated in the same way as the default translation. I also added a test that verifies the changes that I did.

juampynr’s picture

  1. +++ b/src/EntityOperations.php
    @@ -73,9 +73,11 @@ class EntityOperations {
    +        if ($entity->isDefaultTranslation() || $entity->moderation_state->entity->isPublishedState() || $entity->isPublished()) {
    

    First condition is needed so you can add a translation.

    Second condition is needed when you want to publish a translation.

    Third condition is needed when you want to create a new draft for a published translation.

    I am sure that this can be simplified but I don't know enough yet the Content Translation and Workbench Moderation APIs.

  2. +++ b/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -218,6 +221,10 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
    +      if ($translatable) {
    +        $button['#value'] .= ' ' . t('(this translation)');
    

    Core does this when content_translation is enabled. It makes sense to me to add it to make it clear that this change will affect only to this translation.

Crell’s picture

Status: Needs review » Needs work
  1. +++ b/src/EntityOperations.php
    @@ -73,9 +73,11 @@ class EntityOperations {
    +        if ($entity->isDefaultTranslation() || $entity->moderation_state->entity->isPublishedState() || $entity->isPublished()) {
    

    This conditional should be split off to a utility method whose name explains what it means/is doing.

  2. +++ b/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -218,6 +221,10 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
    +      if ($translatable) {
    +        $button['#value'] .= ' ' . t('(this translation)');
    +      }
    

    I'm pretty sure this is a not-right way of building the string. The entire string template needs to be a contiguous string inside of a t(). Which means probably doing

    if ($translatable) { 
      $button['#value'] = t(..., []);
    }
    else {
      $button['#value'] = t(..., []);
    }
    

    And just accepting the duplication. (I've been yelled at for this before. :-) )

  3. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,164 @@
    +class ModerationLocaleTest extends WebTestBase {
    

    Is there a reason this test isn't using the ModerationStateTestBase base class like everything else?

  4. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,164 @@
    +    $this->drupalCreateContentType(array('type' => 'article', 'name' => 'Article'));
    

    WBM uses [] exclusively, as should all of Drupal 8. :-)

  5. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,164 @@
    +    $this->assertEqual($french_node->moderation_state->getValue()['0']['target_id'], 'published');
    

    $french_node->moderation_state->target_id. (Here and elsewhere.)

  6. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,164 @@
    +    $this->drupalPostForm('fr/node/' . $english_node->id() . '/edit', $edit, t('Save and Create New Draft (this translation)'));
    +    $this->assertText(t('Article New draft of translated node has been updated.'));
    +    $this->rebuildContainer();
    +    $english_node = $this->drupalGetNodeByTitle('Another node');
    

    Why is the container rebuild needed? We're just saving a node. If the container needs to be rebuilt after a node is saved, that's a core critical. :-)

juampynr’s picture

Thanks for the feedback. Here is a new patch with its suggestions.

juampynr’s picture

  1. +++ b/src/EntityOperations.php
    @@ -73,16 +73,17 @@ class EntityOperations {
    +        if ($this->entityRevisionOrStateNeedsAdjusting($entity)) {
    

    Couldn't find a better name.

  2. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,158 @@
    +    $english_node = $this->drupalGetNodeByTitle('English node', TRUE);
    

    Discovered that the method has an optional flag to reset the entity's cache. This is much better than rebuilding the container.

Crell’s picture

  1. +++ b/src/EntityOperations.php
    @@ -110,4 +111,18 @@ class EntityOperations {
    +  /**
    +   * Check if an entity's default revision and/or state needs adjusting based on
    +   * its moderation data.
    +   *
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   *
    +   * @return bool
    +   *   TRUE when either the default revision or the state needs to be updated.
    +   */
    +  public function entityRevisionOrStateNeedsAdjusting(EntityInterface $entity) {
    

    Yeah, we can do better than this name. :-)

    In context, what this is determining is whether or not preSave() should be called. Which... seems odd, no? Why would we only sometimes fire the preSave() for an entity's handler? Rather, it's what's in the preSave() that may or may not be needed, but that doesn't necessarily imply preSave() should be blocked.

    Would this logic make more sense in the handlers?

  2. +++ b/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -210,6 +210,9 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
         foreach ($options as $id => $label) {
           $button = [
             '#value' => t('Save and @transition', ['@transition' => $label]),
    @@ -218,6 +221,10 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
    
    @@ -218,6 +221,10 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
             '#weight' => -10,
           ];
     
    +      if ($translatable) {
    +        $button['#value'] = t('Save and @transition (this translation)', ['@transition' => $label]);
    +      }
    

    For cleanliness, I'd prefer to not declare #value in the first place, then set it to one value or the other afterward based on $translatable.

    Actually, the idiomatic way we've been writing code in this module would be:

    $button['#value'] = $translatable
      ? t(...)
      : t(...);
    
juampynr’s picture

Here is an updated patch with @Crell's latest suggestions.

juampynr’s picture

  1. +++ b/src/Entity/Handler/ModerationHandlerInterface.php
    @@ -23,12 +23,8 @@ interface ModerationHandlerInterface {
    -  public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state);
    

    @Crell, I removed this from the interface because we have that info within the entity. Should I revert back to how it was before? I can see why you may want this info to be explicitly passed onto the method but I wonder why.

  2. +++ b/src/Entity/Handler/NodeModerationHandler.php
    @@ -21,12 +21,14 @@ class NodeModerationHandler extends ModerationHandler {
    +    if ($entity->isDefaultTranslation() || $entity->moderation_state->entity->isPublishedState() || $entity->isPublished()) {
    

    This is now a very specific case. Should we write an utility method for these conditions or is it fine to leave them here?

  3. +++ b/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -210,14 +210,21 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
    +      $button['#value'] = $translatable
    

    This now looks very readable :-D

Crell’s picture

Status: Needs review » Needs work
  1. +++ b/src/Entity/Handler/NodeModerationHandler.php
    @@ -21,12 +21,14 @@ class NodeModerationHandler extends ModerationHandler {
    +    if ($entity->isDefaultTranslation() || $entity->moderation_state->entity->isPublishedState() || $entity->isPublished()) {
    

    Yep, this should get split off to its own method. I'm tempted to say it goes on the parent class, but isPublished() is a node-specific thing so probably not.

    My general rule these days is that any multi-part conditional should be split off to a separate method which is pretty much just the same thing (return $a || $b || $c), because then the method name becomes documentation and forces thinking through what we actually want out of the conditional.

    In this case, my knee-jerk thought is shouldPublish($entity). However... That actually indicates this logic is wrong. Because we want to update the published value even if the moderation state is not published. Otherwise archiving won't work. :-)

    This is also a very long conditional primarily because the default revision and published states are being removed from the method, which to me indicates that they shouldn't be. If those are kept, then the check here simply becomes if (is default translation), and then we can add a comment as to why that's important.

    So let's do that, and revert the change to preSave().

  2. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,158 @@
    +    // Enable moderation on Article node type.
    +    $edit = [
    +      'enable_moderation_state' => TRUE,
    +      'allowed_moderation_states[draft]' => TRUE,
    +      'allowed_moderation_states[published]' => TRUE,
    +      'default_moderation_state' => 'draft',
    +    ];
    +    $this->drupalPostForm('admin/structure/types/manage/article/moderation', $edit, t('Save'));
    +    $this->assertText(t('Your settings have been saved.'));
    

    This can/should use the createContentTypeFromUI() method on the parent, no? (Also the line from the setUp() method.)

  3. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,158 @@
    +    $this->assertEqual($english_node->isPublished(), TRUE);
    +    $this->assertEqual($french_node->moderation_state->target_id, 'draft');
    +    $this->assertEqual($french_node->isPublished(), FALSE);
    

    Minor nit: Some of these can use assertTrue()/assertFalse() instead. (Same for a couple of other assertions in this method.)

  4. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,158 @@
    +    $this->assertEqual($french_node->getTitle(), 'Translated node', t('The default revision of the published translation remains the same.'));
    

    Assertion messages in tests shouldn't use t(). (Core is not entirely consistent in this regard, I think, but they're not supposed to use t() to avoid weird and hard to handle dependencies.) Same elsewhere in this method.

juampynr’s picture

Here is an updated version of the patch with the suggestions at #21.

juampynr’s picture

  1. +++ b/src/Entity/Handler/NodeModerationHandler.php
    @@ -52,4 +54,21 @@ class NodeModerationHandler extends ModerationHandler {
    +    return $entity->isDefaultTranslation() || $entity->moderation_state->entity->isPublishedState() || $entity->isPublished();
    

    We still need these three. If you remove any of them, ModerationLocaleTest will fail.

  2. +++ b/src/Plugin/Validation/Constraint/ModerationStateValidator.php
    @@ -85,6 +85,9 @@ class ModerationStateValidator extends ConstraintValidator implements ContainerI
    +    if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) {
    

    This was not letting a translation to be archived since getLatestRevision returns the original entity (not the translated one).

  3. +++ b/src/Tests/ModerationLocaleTest.php
    @@ -0,0 +1,168 @@
    +    $this->drupalPostForm('node/' . $english_node->id() . '/edit', [], t('Save and Archive (this translation)'));
    

    I added these to test archiving both the original and the translation.

Status: Needs review » Needs work

The last submitted patch, 22: moderated_content-2654972-22.patch, failed testing.

The last submitted patch, 22: moderated_content-2654972-22.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
juampynr’s picture

I guess that the above failures are related to the testbot and not my patch.

juampynr’s picture

Status: Needs review » Needs work

Discussed with @Crell in IRC to work on the following:

* Test if the patch works with custom blocks.
* Restore the onPresave signature.

juampynr’s picture

It looks like we won't be able to fix block support as I found a set of unrelated bugs while testing this module with custom blocks. I have created the issue #2665822: Add tests for block moderation behavior to start a different discussion there.

I think that in the short term we should just restore the onPresave signature for this patch and commit it. Then move on to fix the custom block support. I will adjust the patch now.

juampynr’s picture

juampynr’s picture

+++ b/src/Plugin/Validation/Constraint/ModerationStateValidator.php
@@ -85,6 +85,9 @@ class ModerationStateValidator extends ConstraintValidator implements ContainerI
+    if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) {

The second check is needed or an error will be thrown when adding a translation since the next line calls getTranslation(). ModerationLocaleTest crashes if the condition is removed as it performs this action.

  • Crell committed 568eef1 on 8.x-1.x authored by juampynr
    Issue #2654972 by juampynr, Crell: Moderated content cannot be...
Crell’s picture

Status: Needs review » Fixed

OK, I think we're finally done here. Thanks, Juampy! Committed and pushed.

Status: Fixed » Closed (fixed)

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