Convert path.module form alters to a field widget

If we going to make it configurable field that we should care about cardinality then better to reopen #1751210-125: Convert URL alias form element into a field and field widget
The path module can provide widget and alter field info by allowing widget use.

Tests should use now path[0][alias]

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
@@ -53,7 +53,7 @@
-    $this->fieldTypes = $field_type_manager->getConfigurableDefinitions();
+    $this->fieldTypes = $field_type_manager->getDefinitions();

needed here

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
@@ -0,0 +1,87 @@
+  public function errorElement(array $element, ConstraintViolationInterface $violation, array $form, array &$form_state) {
+    return $element['path'];

There's no such thing as $element['path'] ?

I agree with doing baby steps. The original "Make Path a field" issue pretty much failed, because I tried to change too many things at once :P

Fixing the primary test failures revealed some more bugs:

1. Fixed bogus error element key.
2. Updated Path module tests to use new form input name.
3. Moved form element validation handler into PathWidget.
4. Fixed inconsistent field labels.
5. Fixed path field is not marked as translatable.

@sun I suggest you to add validation constraint as we do for fields like #2002168: Convert form validation of terms to entity validation

1. Reverted bogus FieldInfo assertion in PathLanguageTest.
2. Fixed Path JavaScript summary.
3. Removed seemingly obsolete langcode hack/override from form element validation handler.
4. Simplified/fixed langcode handling in PathWidget to match Path CRUD service logic.

I'm not able to make PathLanguageTest work — the original English alias somehow seems to be gone after saving the French alias. I wasn't able to figure out why that is.

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -141,4 +141,30 @@ public function delete($conditions) {
+    $query = db_select('url_alias')

Injection.

1. Fixed only node form is supposed to have a "URL path settings" details wrapper.
2. Use the injected database connection in Path::isUnique().

1) Fix #16
2) fix broken test - interdiff2.txt
3) the following addressed

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
@@ -0,0 +1,96 @@
+      $conditions = array(
+        'source' => $entity->getSystemPath(),
+        'langcode' => $entity->language()->id,

+++ b/core/modules/path/path.module
@@ -91,130 +88,24 @@ function path_menu_link_defaults() {
-    $conditions = array('source' => 'node/' . $node->id());
-    if ($node->language()->id != Language::LANGCODE_NOT_SPECIFIED) {
-      $conditions['langcode'] = $node->language()->id;
-    }

Currently Drupal\path\Tests\PathLanguageTest is broken some how, probably the order of alters with content translation...
The node body field does not store translatable text, and language is not properly passed to new widget

Seems we need Gabor here

I don't think I could be in any way better authority than sun, Berdir and yched combined here :)

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
@@ -0,0 +1,103 @@
+      // Node language needs special care. Since the language of the URL alias
+      // depends on the node language, and the node language can be switched
+      // right within the same form, we need to conditionally overload the
+      // originally assigned URL alias language.
+      // @todo Remove this after converting Path module to a field, and, after
+      //   stopping Locale module from abusing the content language system.
+      if (isset($form_state['values']['langcode'])) {
+        $form_builder->setValue($element['langcode'], $form_state['values']['langcode'], $form_state);

This is a ugliest hunk, any idea how to get rid of it?

23: 2201051-path-widget-22.patch queued for re-testing.

Patch removes @todo, and changes path language to $items->getLangcode() as #23 does

Let's get @plach review on it!
Yes, we need to enable both bundles because of comment in content_translation_entity_base_field_info_alter()

    // Currently field translatability is defined per-field but we may want to
    // make it per-instance instead. In that case, we will need to implement
    // hook_bundle_field_info_alter() instead.

Suppose related issue is #2111887: Regression: Only title (of base fields) on nodes is marked as translatable

re-roll (library, details-open)

1. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
   @@ -0,0 +1,102 @@
   +    $element['pid'] = array('#type' => 'value', '#value' => $path['pid']);
   +    $element['source'] = array('#type' => 'value', '#value' => $path['source']);
   +    $element['langcode'] = array('#type' => 'value', '#value' => $path['langcode']);
   

   multi line please.

2. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
   @@ -0,0 +1,102 @@
   +   * Form element validation handler for URL alias form element.
   

   @param doc missing.

3. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
   @@ -0,0 +1,102 @@
   +  public static function validateFormElement(&$element, &$form_state) {
   

   I think we have to add array before both the params

Hm. This seems to work... content_translation_entity_field_info_alter() is messed up, it doesn't correctly merge by-bundle settings of base field translatability. The first bundle just wins. That stuff is pretty messed up :(
I hoped that #2114707: Allow per-bundle overrides of field definitions would help a bit, but it doesn't.

I also hoped #2114707: Allow per-bundle overrides of field definitions would help :)
Not sure whether that code was written making the wrong assumptions or it's just wrong. Anyway I think it's ok to set path fields as translatable, as written also below.

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
@@ -0,0 +1,102 @@
+      // Entity language needs special care. Since the language of the URL alias
+      // depends on the entity language, and the entity language can be switched
+      // right within the same form, we need to conditionally overload the
+      // originally assigned URL alias language.
+      // @see \Drupal\content_translation\ContentTranslationController::entityFormAlter()

I couldn't find the part the the @see refers to. Anyway we might be able to simplify this code by using $form_state['controller']->getFormLangcode(), but it seems the implemented logic is needed in the isUnique() call below.

+++ b/core/modules/path/path.module
@@ -203,8 +98,13 @@ function path_form_taxonomy_term_form_alter(&$form, $form_state) {
+      ->setTranslatable(TRUE)

I think it's correct to set this translatable as path natively supports language.

Fix for #29

#30 1) ContentTranslationController::entityFormAlter() consumes $form['langcode'] that is set here

There's no ConfigField* after #2192259: Remove ConfigField.. Item and List classes + interfaces

re-roll

fix wrong merge

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldWidget/PathWidget.php
@@ -0,0 +1,116 @@
+      $form_builder = \Drupal::formBuilder();
+      $form_builder->setValue($element['alias'], $alias, $form_state);
+
+      // Entity language needs special care. Since the language of the URL alias
+      // depends on the entity language, and the entity language can be switched
+      // right within the same form, we need to conditionally overload the
+      // originally assigned URL alias language.
+      // @see \Drupal\content_translation\ContentTranslationController::entityFormAlter()
+      if (isset($form_state['values']['langcode'])) {
+        $form_builder->setValue($element['langcode'], $form_state['values']['langcode'], $form_state);
+      }
+

I still think this whole block of code could be simplified. Probably even $entity->language()->id might work as an argument to ::isUnique().

but this code set language for the whole node form!?
@sun suggest to file follow-up , here it is #2225977: Remove langcode manipulation by Path widget

Ok with the follow-up from me, I posted a comment there.

FWIW, the change to use setTranslatable(TRUE) for the path field definition appears to be in line with the changes that were performed by #2111887: Regression: Only title (of base fields) on nodes is marked as translatable — so hopefully that question is obsolete now.

Didn't study the latest patch yet.

re-roll, isUnique() replaced with aliasExists() in 'path.alias_storage'

patch

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
@@ -55,10 +57,7 @@ public function insert() {
-      // Ensure fields for programmatic executions.
-      $langcode = $entity->language()->id;
-
-      if ($path = \Drupal::service('path.crud')->save($entity->getSystemPath(), $this->alias, $langcode)) {
+      if ($path = \Drupal::service('path.crud')->save($entity->getSystemPath(), $this->alias, $this->getLangcode())) {

@@ -76,10 +75,7 @@ public function update() {
-      // Ensure fields for programmatic executions.
-      $langcode = $entity->language()->id;
-
-      \Drupal::service('path.crud')->save($entity->getSystemPath(), $this->alias, $langcode, $this->pid);
+      \Drupal::service('path.crud')->save($entity->getSystemPath(), $this->alias, $this->getLangcode(), $this->pid);

not included in last patch

revert this change

forget to change isUnique()

Another re-roll

Looks good to me, thanks!

This looks like good clean-up, although looks like we have work still to do to remove the form alters and/or have this configurable as part of the form display.

Committed/pushed to 8.x., thanks!

@Berdir correct me if I'm wrong but don't aren't form display modes the UI for vertical tabs stuff on entity forms?

@alexweber, nope, form modes just provide the option to arrange fields differently in various forms.

After this patch: When adding an entity reference field to an article via the UI, and saving the field settings (I used target entity of: content) I get the following:
Notice: Undefined index: columns in Drupal\Core\Field\FieldDefinition->getSchema() (line 484 of core/lib/Drupal/Core/Field/FieldDefinition.php).
I think it has something to do with pathItem being a "computed field" and not having a schema.

Yes I found that bug in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities as well. The problem is that getSchema () should initialize columns to an empty array by default. See the latest patch there for example.

Here's the relevant hunk of that patch extracted. When I found this I didn't realize that this means that adding fields from the UI is broken. That makes this a critical bug. Instead of reverting this let's just commit the attached patch and add test coverage in a follow-up?

Sorry, complete brain failure. Don't know why I thought of Entity Reference.

Looks good to me.

That internal Field API code wasn't touched by this issue/patch here, so that's a preexisting problem and the change of this issue is not guilty for that.

Created #2257769: Adding an Entity Reference field in the Field UI throws a PHP notice; fails to add field