Apply formatters and widgets to Shortcut base fields

This patch iterates on #1 fixing the tests to reflect the changes. I pair programmed this with @rvilar.

This looks good to me...

Thank you guys!

One small detail:

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
@@ -150,9 +150,21 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setDescription(t('The name of the shortcut.'))

The current form has no description below the name field and this description doesn't add any value. It should be removed.

@ e0ipso: I'm un-assigning this issue for now, but if you're still actively working on it, please feel free to re-assign it.

I'm still working on it. I am removing the form validation in favor of a custom constraint. Please, consider this re-assigned back to myself.

Just a thought - Perhaps it's easier to get your changes committed if you split it into two separate issues, one for the conversion to base-fields-with-formatters, another for reworking the validation.

I'm fairly sure that @amateescu wants to use the link field type for this as soon as that is in, I wouldn't waste any time on the current path related fields.

I applied formatters to the path field and added the field_prefix and field_suffix settings to the StringItem. I run into some challenges due to the fact that the path field is computed and therefore was not being displayed in the entity form by default and the value was being cleared since computed fields are considered to be empty.

As per the validation I removed the dependency on the form, provided that we should be using constraints for this -as agreed with @Wim Leers- in Szeged. To do so I had to declare a new custom constraint and fix a bug where empty keys were not being processed appropriately (in my understanding of PHP $array[''] should be the same as $array[NULL]).

I hope this constraint serves as an example for other issues needing to validate the field values. Finally I added a test to assert the failure when a Shortcut link tries to use a path that doesn't exist.

As per the previous comment suggesting to remove the ->setDescription(…), I'm not sure that is such a good idea since we would be deleting the description from the field definition just to remove the markup from the widget. If it's that important we can alter the form element in the Shortcut::form(), I you feel that is worth it I'll be glad to provide a new patch with that. Also https://drupal.org/comment/8623909#comment-8623909 may be relevant to this.

PS: @Berdir, I did this on my plane back to home, so I already wasted my time… :-( Also I was under the impression that the link field did not support internal links yet, but I may be wrong.

The issue I meant is #2054011: Implement built-in support for internal URLs, you are completely correct about not supporting internal path but that issue will change it. And then Shortcut will change quite a bit.

Updated patch to remove custom constraint for the validation part.

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
   @@ -124,7 +139,7 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   -    $url = Url::createFromPath($this->path->value);
   +    $url = Url::createFromPath($this->get('path')->value);
   

   $this->getPath(), then ?

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
   @@ -186,4 +225,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +  public function getProperties($include_computed = FALSE) {
   +    $properties = parent::getProperties($include_computed);
   +    $properties['path'] = $this->get('path');
   +    return $properties;
   +  }
   

   Looks a bit weird, is that temporary pending refactorings in another issue ?
   In that case a @todo comment pointing to the issue would be nice.

   At any rate, would deserve a comment expliciting that this exposes the 'path' field that would otherwise be left out by ContentEntityBase::getProperties() since it's 'computed', and most of all *why* 'path' needs to be part of getProperties().

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutPathItem.php
   @@ -27,4 +27,23 @@ public static function propertyDefinitions(FieldDefinitionInterface $field_defin
   +    foreach ($this->properties as $property) {
   +      if ($property->getValue() !== NULL) {
   +        return FALSE;
   +      }
   +    }
   +    if (isset($this->values)) {
   +      foreach ($this->values as $name => $value) {
   +        if (isset($value) && !isset($this->properties[$name])) {
   +          return FALSE;
   +        }
   +      }
   +    }
   

   Looks overly verbose, other isEmpty() methods in other FieldItem classes do not bother with searching both $this->properties and $this->values, they just use $this->get($property) or directly the $this->{$property} shortcut.

Looks overly verbose, other isEmpty() methods in other FieldItem classes do not bother with searching both $this->properties and $this->values, they just use $this->get($property) or directly the $this->{$property} shortcut.

Actually it was only needed in order to the provided value to be passed along when using constraint based validations. Since I completely got rid of it (as per @Berdir suggestion) this method should not be needed anymore.

Thanks !

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
@@ -187,4 +226,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+  public function getProperties($include_computed = FALSE) {
+    // Manually force the path to be returned since computed fields are not. We
+    // need this for the path form element to be rendered in the entity form by
+    // \Drupal\Core\Entity\Display\EntityFormDisplay::buildForm().
+    $properties = parent::getProperties($include_computed);
+    $properties['path'] = $this->get('path');

Ok, then what puzzles me is why the "path" field is computed while it has a widget to make it editable.
"Editable by user input" and "computed" seem mutually exclusive ?

@yched I *think* that we initially had a computed field that populated the custom path form element.

The real fields are the ones with the route information, not the path, but since it's cumbersome to have users adding route names and such there was this helper form element that generated that info from a path. To populate that form element we were using the value from the computed field. The only thing the computed element did was generate a path based on the route information.

So we went from a custom form element that generated the route info + a computed field that generated the path from the route to a computed field that shows its widget in the entity form.

Does that make any sense?

BTW: I did not expect this many intricacies in a novice issue :-P, although I have to admit it was fun playing with custom constraint validators.

Yeah, the Novice part about this issue for me was fixing the existing tests :)

Again, I really think we shouldn't waste time on the path stuff in the current state, it will change anyway. #2054011: Implement built-in support for internal URLs is RTBC now and a beta blocker, so hopefully it will get in soon and then we can have a follow-up issue to refactor shortcut to use it and use that widget, which should then more or less just work.

@Berdir, this seems to contradict #7. Is the patch in #4 good then? We may need to re-test it then.

IMHO yes, we should go back to #4. The link issue has been committed in the meantime, so we can start to use it, but I don't think we should do it here. So either convert it first and postpone this issue on it or just get this in without the path and then use the widget form the start in the other issue.

What can be further cleaned up in #4 are the #type value form elements there, they are useless.

The following patch is the same as in #4 without the #type value form elements, as suggested by @Berdir.

+1 from me to move forward with this, someone else wants to set it back to RTBC as it's based on my suggestion?

I think it's fine to move forward with this as-is and leave the 'path' part to another issue that makes shortcuts use the improved link field.

Btw, I started doing exactly that in #2235457: Use link field for shortcut entity. This might even remove the separate title field depending on what we want, but there are quite a number of bugs related to the link field that need to be fixed before that can happen, so let's get this in while working on that.

Committed b0fb47f and pushed to 8.x. Thanks!