Disallow saving invalid non-default revisions
Closes #3499181
Merge request reports
Activity
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.."); changed this line in version 5 of the diff
added 19 commits
-
8720d936...1630acb8 - 15 commits from branch
project:11.x
- bb358c38 - disallow saving invalid non-default revisions
- 7cd0f8a6 - add missing check
- 7e3cd513 - move revision check
- 14876bff - Allow resaves of revisions that used to be the default
Toggle commit list-
8720d936...1630acb8 - 15 commits from branch
added 1 commit
- 31304fc1 - Ensure boolean return value for isDefaultRevision()
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
Toggle commit list-
31304fc1...87982857 - 3 commits from branch
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 thenew_value
is true the property will be true if it's not a new. So you can have a case whereisDefaultRevision
isFALSE
, and$new_value
isTRUE
thenisDefaultRevision
will be set toTRUE
, but the method will still returnFALSE
. 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 nicxvanfrom 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 nicxvanchanged this line in version 13 of the diff
- Resolved by Sascha Grossenbacher
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.
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.
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()); 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.
added 1 commit
- 3f1d183a - Do not use toUrl()->toString() as this does not work within a subdirectory
- Resolved by Andrei Mateescu
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') so when you edit a pending draft with a published default revision that does not have a menu link yet and then you add one, this now saves that as a menu link to the latest URL. That means when we look up existing menu links for this node, we always also need to look for both possible targets.
changed this line in version 13 of the diff
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
Toggle commit list-
6e752379...999063c1 - 176 commits from branch
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
Toggle commit list-
bd63c718...6dd918c9 - 5 commits from branch
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
Toggle commit list-
ec605a2d...75ea9ab7 - 8 commits from branch
- Resolved by Sascha Grossenbacher
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'; Ah the other instance has the comment, I knew I asked for a comment on this behavior and @berdir had added it.
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
Toggle commit list-
08b1f08b...4a3fbdbb - 40 commits from branch
added 21 commits
-
409709f9...ec7c5ca6 - 19 commits from branch
project:11.x
- 42baa9fc - Merge branch '11.x' into 3499181-disallow-saving-the
- eac57a7d - Add a @todo for a followup.
-
409709f9...ec7c5ca6 - 19 commits from branch