Skip to content
Snippets Groups Projects

Disallow saving invalid non-default revisions

Closes #3499181

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
758 758
759 759 $id = parent::doPreSave($entity);
760 760
761 if ($entity->isNew() && !$entity->isDefaultRevision()) {
762 throw new EntityStorageException("A new entity of type '{$this->entityTypeId}' must be created as the default revision..");
  • added 19 commits

    Compare with previous version

  • added 4 commits

    • 38aea2db - fix extra .
    • ba8e8c99 - update migrate tests to not change default revision
    • 803d9c44 - update revision test to except an exception
    • 72ce5100 - Fix menu_ui and content_moderation integration by pointing new draft links to the latest route

    Compare with previous version

  • added 1 commit

    • 31304fc1 - Ensure boolean return value for isDefaultRevision()

    Compare with previous version

  • added 13 commits

    • 31304fc1...87982857 - 3 commits from branch project:11.x
    • 5b184865 - disallow saving invalid non-default revisions
    • 648c63b5 - add missing check
    • cfeb9253 - move revision check
    • 5ecc1458 - Allow resaves of revisions that used to be the default
    • b3729d7d - fix extra .
    • ab1aaa36 - update migrate tests to not change default revision
    • e17ad125 - update revision test to except an exception
    • 07c2a21d - Fix menu_ui and content_moderation integration by pointing new draft links to the latest route
    • 9757d4e9 - Ensure boolean return value for isDefaultRevision()
    • ea4ae117 - fix EntityRevision and isDefaultRevision

    Compare with previous version

  • added 1 commit

    • eb441b55 - remove duplicate wasDefautRevisionCheck()

    Compare with previous version

  • 346 346 */
    347 347 public function isDefaultRevision($new_value = NULL) {
    348 348 $return = $this->isDefaultRevision;
    349 if (isset($new_value)) {
    350 $this->isDefaultRevision = (bool) $new_value;
    351 }
    352 349 // New entities should always ensure at least one default revision exists,
    353 350 // creating an entity without a default revision is an invalid state.
    354 return $this->isNew() || $return;
    351 if (isset($new_value) && (!$this->isNew() || $new_value === TRUE)) {
    352 $this->isDefaultRevision = (bool) $new_value;
    353 }
    354 return (bool) $return;
    • The problem here was that you could call setNewRevision(FALSE) on a new entity, then it still returned TRUE until saved, and in postSave() it suddenly no longer was the defaultRevision(). menu_ui actively relied on this behavior to cheat about creating new "non-default" menu links that didn't end up in menu storage at first.

      so I moved the isNew() check up to disallow changing it to FALSE. That resulted in a new edge case when duplicating a non-default revision, because the internal flag is already FALSE and we need to allow setting it to TRUE in createDuplicate (or we could set the property directly).

      Maybe it's more self-explanatory to say if ((!$entity->isNew() && $new_value === FALSE) || $new_value == TRUE)? or maybe trying to set it to FALSE on a new entity should even throw an exception instead of silently ignoring that request?

    • Sorry for this comment / question in advance, just trying to understand how this bit works.

      I'm really not a fan of using $return as a variable name, it makes parsing this so much harder.

      Looking at this full function in context:

      public function isDefaultRevision($new_value = NULL) {
          $return = $this->isDefaultRevision;
          // New entities should always ensure at least one default revision exists,
          // creating an entity without a default revision is an invalid state.
          if (isset($new_value) && (!$this->isNew() || $new_value === TRUE)) {
            $this->isDefaultRevision = (bool) $new_value;
          }
          return (bool) $return;
        }

      It reads to me now that it will return whatever was in $this->isDefaultRevision at the time of the call, however if the new_value is true the property will be true if it's not a new. So you can have a case where isDefaultRevision is FALSE, and $new_value is TRUE then isDefaultRevision will be set to TRUE, but the method will still return FALSE. Is that the expected behavior. (BTW the property and function being called the same makes this tough to parse too).

      Does this cause problems with translations since the property is set by reference for translations?

      Edited by nicxvan
    • from the interface:

         * @return bool
         *   TRUE if the entity is the default revision, FALSE otherwise. If
         *   $new_value was passed, the previous value is returned.

      So yes, this the existing behavior and I'm not changing that, just making sure that it actually does behave correctly and as documented in edge cases.

      but fully agreed that this is a very confusing method being both a getter and a setter. there are existing issues about that. could change $return to $current_value or something, but this isn't really changed here.

    • Thanks! To be clear, I am not suggesting changing this logic, just making sure I parsed it correctly.

      I would rename $return to $current_value since that always bites me.

      Edited by nicxvan
    • Sascha Grossenbacher changed this line in version 13 of the diff

      changed this line in version 13 of the diff

    • Please register or sign in to reply
  • 177 177 $this->drupalGet('node/' . $node->id() . '/edit');
    178 178 $this->submitForm($edit, 'Save');
    179 179 $this->assertSession()->pageTextContains("Page {$node->label()} has been updated.");
    180
    181 // The link is created to the latest page, which the editor is allowed
    182 // see, but an anonymous visitor not.
    183 $this->assertSession()->linkExists('Second test menu link');
    184 $this->drupalLogout();
    180 185 $this->assertSession()->linkNotExists('Second test menu link');
    181 186
    187 $this->drupalLogin($editor);
    • This is a deliberate behavior change. What this originally attempted to do is impossible. A new menu link can't be a draft, it relied on the bug in isDefaultRevision() to "work". See #3485030 for more too where this was discovered. The workaround here is that we create the new menu link and point it to node/ID/latest, so that it's only visible to users who are allowed to see that and retarget it on publish of the node back to /node/ID. that requires that queries to look up menu links need to look for menu links to either URL.

    • That must have been fun to track down.

    • While I don't love that workaround, I agree that it's only sane thing to do at the moment, at least until Content Moderation is changed to use Workspaces under the hood and a proper pending revision can be created for the menu item without altering its link.

    • Please register or sign in to reply
  • 48 48 $entity->menu_name->value = $values['menu_name'];
    49 49 $entity->parent->value = $values['parent'];
    50 50 $entity->weight->value = $values['weight'] ?? 0;
    51 $entity->isDefaultRevision($node->isDefaultRevision());
    51 if ($entity->isNew()) {
    52 if (!$node->isDefaultRevision() && $node->hasLinkTemplate('latest-version')) {
    53 $entity->get('link')->uri = 'internal:' . $node->toUrl('latest-version')->toString();
    54 }
    55 }
    56 else {
    57 $entity->isDefaultRevision($node->isDefaultRevision());
    58 if (!$entity->isDefaultRevision()) {
    59 $entity->setNewRevision(TRUE);
    • the second relevant part here is that we ensure that a non-default revision of the menu link is saved as a new revision. For that to work, we need to update the load() calls with getActive() so that it returns the correct pending revision. there are likely still some edge cases around multiple translations and correctly merging them together, but that is a separate topic not directly related to this.

    • Please register or sign in to reply
  • 158 163
    159 164 $entity->enforceIsNew(FALSE);
    160 165 $entity->setNewRevision(TRUE);
    166 $entity->isDefaultRevision(FALSE);
    161 167 }
    162 168 // We need to update the entity, so that the destination row IDs are
    163 169 // correct.
    164 170 $entity = $this->updateEntity($entity, $row);
    165 $entity->isDefaultRevision(FALSE);
  • 160 163 $this->assertTrue($entity2->wasDefaultRevision());
    161 164 $entity2->isDefaultRevision(FALSE);
    162 165 $entity2->save();
    163 166 $this->assertTrue($entity2->wasDefaultRevision());
    • this was attempting to test this bug but it was incomplete and only tested one specific aspect and not the data loss that actually happens. I changed it to expect an exception, that means it actually never reaches this final assertion anymore.

    • Please register or sign in to reply
  • 281 281 // The last call changed the return value for this call.
    282 282 $this->assertFalse($this->entity->isDefaultRevision());
    283 283 // The revision for a new entity should always be the default revision.
    284 $this->entity->isDefaultRevision(TRUE);
    284 285 $this->entity->expects($this->any())
    • this is a unit test that's messing with internals. similar problem as createDuplicate(), it makes an entity "new" that is already set to not being a default revision earlier. So we have to reset the state to to being a default revision to ensure that it then can't be set as not being default revision.

    • Please register or sign in to reply
  • added 1 commit

    • 3f1d183a - Do not use toUrl()->toString() as this does not work within a subdirectory

    Compare with previous version

  • nicxvan
  • 74 87 if (in_array($menu_name, $type_menus)) {
    75 88 $query = \Drupal::entityQuery('menu_link_content')
    76 89 ->accessCheck(TRUE)
    77 ->condition('link.uri', 'entity:node/' . $node->id())
    90 ->condition('link.uri', ['entity:node/' . $node->id(), 'internal:/node/' . $node->id() . '/latest'], 'IN')
  • added 1 commit

    • 6e752379 - use local variables to document behavior

    Compare with previous version

  • added 190 commits

    • 6e752379...999063c1 - 176 commits from branch project:11.x
    • 999063c1...bde7cf7b - 4 earlier commits
    • 496d699a - fix extra .
    • f4c7e685 - update migrate tests to not change default revision
    • 8730a0bc - update revision test to except an exception
    • 3d2f9907 - Fix menu_ui and content_moderation integration by pointing new draft links to the latest route
    • 6ab5194b - Ensure boolean return value for isDefaultRevision()
    • 81faf99d - fix EntityRevision and isDefaultRevision
    • 3e68fad0 - remove duplicate wasDefautRevisionCheck()
    • ebac45ad - Do not use toUrl()->toString() as this does not work within a subdirectory
    • 48946ee7 - use local variables to document behavior
    • bd63c718 - fix typo in suggestin

    Compare with previous version

  • added 20 commits

    • bd63c718...6dd918c9 - 5 commits from branch project:11.x
    • 6dd918c9...8619a74d - 5 earlier commits
    • 83248ed3 - update migrate tests to not change default revision
    • 7f20a203 - update revision test to except an exception
    • 5e0def0a - Fix menu_ui and content_moderation integration by pointing new draft links to the latest route
    • f66789d1 - Ensure boolean return value for isDefaultRevision()
    • c9585a24 - fix EntityRevision and isDefaultRevision
    • 0fdc5b54 - remove duplicate wasDefautRevisionCheck()
    • 98c85f2b - Do not use toUrl()->toString() as this does not work within a subdirectory
    • 49444f6b - use local variables to document behavior
    • 7fe1226c - fix typo in suggestin
    • ec605a2d - Avoid errors when saving new entity

    Compare with previous version

  • added 24 commits

    • ec605a2d...75ea9ab7 - 8 commits from branch project:11.x
    • 75ea9ab7...c06b0f2b - 6 earlier commits
    • 6a2f23e8 - update revision test to except an exception
    • 50259314 - Fix menu_ui and content_moderation integration by pointing new draft links to the latest route
    • 8937426e - Ensure boolean return value for isDefaultRevision()
    • beb3f878 - fix EntityRevision and isDefaultRevision
    • 14c48faa - remove duplicate wasDefautRevisionCheck()
    • 0a6927a8 - Do not use toUrl()->toString() as this does not work within a subdirectory
    • 616f12ca - use local variables to document behavior
    • e97064a4 - fix typo in suggestin
    • 9feafdfb - Avoid errors when saving new entity
    • 08b1f08b - improve comments and variable names

    Compare with previous version

  • catch @catch started a thread on the diff
  • 48 48 $entity->menu_name->value = $values['menu_name'];
    49 49 $entity->parent->value = $values['parent'];
    50 50 $entity->weight->value = $values['weight'] ?? 0;
    51 $entity->isDefaultRevision($node->isDefaultRevision());
    51 if ($entity->isNew()) {
    52 if (!$node->isDefaultRevision() && $node->hasLinkTemplate('latest-version')) {
    53 $entity->get('link')->uri = 'internal:/node/' . $node->id() . '/latest';
  • added 57 commits

    • 08b1f08b...4a3fbdbb - 40 commits from branch project:11.x
    • 4a3fbdbb...9bc83770 - 7 earlier commits
    • 65fc57e2 - Fix menu_ui and content_moderation integration by pointing new draft links to the latest route
    • abfb00b4 - Ensure boolean return value for isDefaultRevision()
    • 59d5fd62 - fix EntityRevision and isDefaultRevision
    • 719bebf1 - remove duplicate wasDefautRevisionCheck()
    • d3a12479 - Do not use toUrl()->toString() as this does not work within a subdirectory
    • e85a5d69 - use local variables to document behavior
    • 944886f7 - fix typo in suggestin
    • f9d7a2bd - Avoid errors when saving new entity
    • eef46cb7 - improve comments and variable names
    • 322fdcd3 - update exception message, more comments

    Compare with previous version

  • added 1 commit

    • 409709f9 - Update expected exception message

    Compare with previous version

  • Andrei Mateescu added 21 commits

    added 21 commits

    Compare with previous version

  • closed

  • Please register or sign in to reply
    Loading