Re-process the user-entered-paths for custom menu links when there is a menu rebuild

small nit to clarify.

Note, without #2416955: Convert MenuLinkContent to use a link widget we can't distinct between entity URIs and user-path URIs.

#2416955: Convert MenuLinkContent to use a link widget landed.

I'll try to tackle it now.

WIP

Alright, there we go.

This could be it, as always, this is also part of some of the other patches.

@dawehner looks like you uploaded the interdiff twice :)

Ha, good catch! Here is the actual patch.

Fair!

1. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -179,6 +180,17 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   +      $this->rediscover->value = TRUE;
   
   @@ -294,6 +306,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['rediscover'] = BaseFieldDefinition::create('boolean')
   +      ->setLabel(t('Indicates whether the menu link should be rediscovered'))
   +      ->setSetting('default_value', FALSE);
   

   Can we add methods to MenuLinkContentInterface for this - and use that instead - makes the code more readable.

   ::setRequiresRediscovery()
   ::requiresRediscovery()?

2. +++ b/core/modules/menu_link_content/src/Plugin/Deriver/UserPathMenuLinkContentDeriver.php
   @@ -0,0 +1,90 @@
   +   * @param \Drupal\Core\Entity\Query\QueryFactory $query_factory
   +   *   The query factory.
   ...
   +    $this->queryFactory = $query_factory;
   

   #2389335: Deprecate entity.query service and replace with using the entity storage's getQuery() method is trying to remove the entity.query service, can we use EntityManger->getStorage($entity_type_id)->getQuery() instead - we already have the entity manager for storage sake.

3. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
   @@ -17,6 +17,7 @@
   + *
   

   Not needed?

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -294,6 +306,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setSetting('default_value', FALSE);
 
+    $fields['rediscover'] = BaseFieldDefinition::create('boolean')
+      ->setLabel(t('Indicates whether the menu link should be rediscovered'))
+      ->setSetting('default_value', FALSE);
+

This apparently re-introduced incorrect default value settings, I think YesCT already openend an issue for the existing ones. there is no such setting, use is setDefaultValue() instead.

Can we add methods to MenuLinkContentInterface for this - and use that instead - makes the code more readable.

::setRequiresRediscovery()
::requiresRediscovery()?

Realized that we can also just adapt the entity query.

#2389335: Remove entity.query service and replace with using the entity storage's getQuery() method is trying to remove the entity.query service, can we use EntityManger->getStorage($entity_type_id)->getQuery() instead - we already have the entity manager for storage sake.

Good point.

Not needed?

Fixed

This apparently re-introduced incorrect default value settings, I think YesCT already openend an issue for the existing ones. there is no such setting, use is setDefaultValue() instead.

Fixed my no longer using it.

.

About 'default_value' : found a couple more in current MenuLinkContent::baseFieldDefinitions()
Patch in #2418117: MenuLinkContent::baseFieldDefinitions() wrongly passes default values as a field setting

Thank you kalpana to manual test it AND add the manual instructions.

I have manually tested the patch and menu_tree table does store different route_name after deleting the views. Reviewing the code and working on patch.

There is no API changes so it doesn't need change records.

The patch is looking wonderfully simple :)

1. +++ b/core/modules/menu_link_content/src/Plugin/Deriver/UserPathMenuLinkContentDeriver.php
   @@ -17,8 +16,8 @@
   + * compare to entity referenced ones.
   

   s/compare/compared/

2. +++ b/core/modules/menu_link_content/src/Plugin/Deriver/UserPathMenuLinkContentDeriver.php
   @@ -0,0 +1,78 @@
   +   * Constructs a TreeRediscoverSubscriber instance.
   

   The class name is wrong; copy/paste remnant.

3. +++ b/core/modules/menu_link_content/src/Tests/TreeRediscoverSubscriberIntegrationTest.php
   @@ -0,0 +1,71 @@
   +    // Ensure that the new route name / parameters are caught up by the tree.
   

   "are caught up" sounds a bit strange.

Also should we be adding an index for the new field for performance sake?

Also should we be adding an index for the new field for performance sake?

I would like to do that, but I could not figure out how to add an index to a base field ... given that the indexes seemed to live on the field type instead.

working on this.

There is no API right now to add indexes for base fields or indexes that span multiple fields. The best you can do right now is visible in FileStorageSchema.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -182,6 +182,16 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
+      $this->rediscover->value = TRUE;

Would still like to see a method for this added to MenuLinkContentInterface, with a setter too

working on #38 and #32

Fixes #38 and #32
Hope I don't break anything

Such a nice, clean, and direct way of solving this!

1. +++ b/core/modules/menu_link_content/menu_link_content.links.menu.yml
   @@ -0,0 +1,4 @@
   +  deriver: \Drupal\menu_link_content\Plugin\Deriver\UserPathMenuLinkContentDeriver
   

   This class has nothing directly to do with user paths. It uses the rediscover field, which contrib can repurpose to cover other use cases too without needing to change the deriver, which is awesome. So, let's name it more generically, like simply MenuLinkContentDeriver. I don't think we even need to prefix with Rediscoverable or anything like that, since that's implicit to what a deriver is.

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -182,6 +183,16 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   +    if (parse_url($this->link->uri, PHP_URL_SCHEME) === 'user-path') {
   +      $this->setRequiresRediscovery(TRUE);
   +    }
   

   What if a path changes from user-path to not (e.g., link is edited and changed from custom path to an entity that was autocompleted)? Should we add a setRequiresRediscovery(FALSE) for that? I think that's only needed when the scheme changes, not every time there's a save.

3. +++ b/core/modules/menu_link_content/src/MenuLinkContentInterface.php
   @@ -102,4 +102,35 @@ public function getWeight();
   +   * provided by a route in views module, but later this path may be served by
   +   * the panels module. Flagging a link as requiring rediscovery ensures that if
   

   Capitalize Views and Panels?

I haven't followed all the comments here. Is there any other unaddressed feedback? If not, I think this is ready once at least item 1 from this comment is done. The other items could be followups if needed.

I think we're ready here - thanks all

Ok great. This looks like what we talked about. Thanks for all the great testing in this issue! Bummer about the index thing, but that's a pre-existing condition.

Committed and pushed to 8.0.x. Thanks so much!!!

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -297,6 +311,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setSetting('default_value', FALSE);

This re-reintroduced the wrong syntax for default values :-p
See #23 and #26

Added it to the fixes in #2418117: MenuLinkContent::baseFieldDefinitions() wrongly passes default values as a field setting...