Problem

  • The current URL alias form widget is hard-coded for nodes and taxonomy terms only.

Goal

  • Allow to have a URL alias on any kind of entity (bundle).
  • Clean up and refactor the code to bring it up to par with current core subsystems.

Proposed solution

  1. Make the URL alias as field + field widget, so it can be applied to any kind of entity (bundle).
  2. If you want URL alias support somewhere, just add the field.
  3. If you do not want URL alias support somewhere, just remove the field.
  4. Pave the way for token + pathauto integration.

Follow up issues

Related issues

Review notes


Original summary

In #1553358: Figure out a new implementation for url aliases we have been discussing what to do about getting the current core url aliasing into CMI. One thing has been pointed out is that the most common use case for this functionality is aliasing individual nodes (for instance to map old urls to new ones when doing a site conversion.) My feeling is that this use case doesn't belong in url aliases at all, it belongs in the node because this data is in fact content specific to a node. So I would like to do the following

- Add a multi-instance text field instance as a default on all new content types for adding aliases to the current node.
- When the node is saved, save these aliases out to somewhere, (a new cache bin maybe?) for peformant reading.
- Set this field to be Format: Hidden by default in all view modes

Making it a field makes a lot of sense, because we have multi-instance handling right there, and I feel like the aliases are worthy of being exposed in the node forms as a first-class citizen. It does, however, open a new core paradigm of adding field instances as metadata (since it is unlikely anyone would ever want to render these out.)

Thoughts? Not sure I'm the one to write the patch since I have precious little experiene with the field and entity system.

Files: 
CommentFileSizeAuthor
#125 Screen Shot 2013-02-18 at 11.33.21 PM.png36.01 KBwebchick
#124 1751210-interdiff-123.txt6.12 KBandypost
#124 1751210-path-123.patch55.36 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1751210-path-123.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#120 1751210-interdiff-120.txt3.02 KBandypost
#120 1751210-path-120.patch55.68 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,815 pass(es).
[ View ]
#118 path.field_.118.patch55.06 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,655 pass(es).
[ View ]
#118 interdiff.txt10.48 KBsun
#116 doubledescription.png22.26 KBjhodgdon
#113 path.field_.113.patch48.05 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,036 pass(es).
[ View ]
#111 path.field_.111.patch48.12 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path.field_.111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#111 interdiff.txt2.09 KBsun
#110 stdinstall.png11.6 KBjhodgdon
#109 path.field_.109.patch48.02 KBsun
PASSED: [[SimpleTest]]: [MySQL] 48,696 pass(es).
[ View ]
#109 interdiff.txt7.19 KBsun
#106 path.field_.106.patch45.38 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,777 pass(es), 25 fail(s), and 3 exception(s).
[ View ]
#106 interdiff.txt1.42 KBsun
#104 path.field_.104.patch44.79 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,673 pass(es), 25 fail(s), and 45 exception(s).
[ View ]
#104 interdiff.txt1.86 KBsun
#103 path.field_.103.patch44.78 KBsun
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#99 path.field_.99.patch44.8 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,670 pass(es), 42 fail(s), and 205 exception(s).
[ View ]
#89 path.field_.89.patch44.84 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path.field_.89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#89 interdiff.txt1014 bytessun
#86 path.field_.86.patch45.22 KBsun
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/path/lib/Drupal/path/Plugin/field/widget/PathWidget.php.
[ View ]
#86 interdiff.txt531 bytessun
#83 path.field_.83.patch44.72 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,307 pass(es).
[ View ]
#83 interdiff.txt4.74 KBsun
#75 path.field_.75.patch43.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 48,837 pass(es).
[ View ]
#69 path.field_.69.patch47.7 KBsun
PASSED: [[SimpleTest]]: [MySQL] 48,818 pass(es).
[ View ]
#69 interdiff.txt5.2 KBsun
#67 path.field_.67.patch46.25 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,734 pass(es), 2 fail(s), and 9 exception(s).
[ View ]
#67 interdiff.txt5.39 KBsun
#65 path.field_.65.patch43.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,756 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#65 interdiff.txt1.67 KBsun
#61 path.field_.61.patch43.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#61 interdiff.txt861 bytessun
#59 path.field_.59.patch42.82 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,722 pass(es), 3 fail(s), and 20 exception(s).
[ View ]
#59 interdiff.txt563 bytesdagmar
#57 path.field_.57.patch42.82 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#57 interdiff.txt5.36 KBsun
#56 path.field_.56.patch42.14 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,807 pass(es), 9 fail(s), and 5,572 exception(s).
[ View ]
#56 interdiff.txt4.88 KBsun
#55 path.field_.55.patch41.4 KBsun
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#49 Screen Shot 2012-11-21 at 6.07.20 PM.png35.76 KBlarowlan
#48 Screen Shot 2012-11-21 at 5.08.11 PM.png10.89 KBlarowlan
#48 Screen Shot 2012-11-21 at 5.08.37 PM.png9.3 KBlarowlan
#45 pathfield.png22.1 KBdagmar
#45 fieldlist.png26.57 KBdagmar
#39 path.field_.39.patch44.64 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,305 pass(es), 33 fail(s), and 2 exception(s).
[ View ]
#39 interdiff.txt24.29 KBsun
#37 path.field_.37.patch22.73 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,147 pass(es), 41 fail(s), and 5 exception(s).
[ View ]
#37 interdiff.txt16.22 KBsun
#30 core--pathauto--1751210--30.diff51.42 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] 48,160 pass(es), 40 fail(s), and 5 exception(s).
[ View ]
#21 path.field_.21.patch42.08 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path.field_.21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 basicpath.gif2.74 KBjhodgdon

Comments

I could actually whip this up fairly quickly. It should be added by default to node, users, and taxonomy terms.

Just talked a little more with davereid about this. We shouldn't save the normalized data out to the existing url_alias table, because if the existing UI for URL aliases remains in core and doesn't get committed to CMI, it will still be clogged with these node aliases. It should at least be to a separate table but I really like the cache bin option. This data should be considered volatile and temporary and cache is the system we have for that.

Field type alias... sounds relatively simple to knock out, path could depend on the the module and add the field to each of the entities listed above when it/the respective modules were enabled

Are you suggesting this as a replacement for the current alias system, or in addition to it?

In addition to. The current alias system would still be necessary because not all aliases lead to content (for instance aliases to Views, the login page, etc.)

OK, I'm good with that. Thanks for the reply.

Status:Active» Closed (duplicate)

It appears this work is being picked up in #229568-25: [META] Pathauto in Core so I'm closing this one.

Component:entity system» path.module
Assigned:Unassigned» sun
Priority:Normal» Major
Status:Closed (duplicate)» Active
Issue tags:+Usability, +Killer End-User Features, +token, +pathauto, +Platform Initiative

Re-opening, since #229568: [META] Pathauto in Core does not want to deal with this aspect, but focus on the "automated" part only.

I have working and almost ready code for this, see #229568-31: [META] Pathauto in Core and following comments for explanations and also screenshots. I will copy that over to this issue in a bit.

Yes, I think there are two separate aspects:
a) )(make it a field) moving the aliases to be a field on the node (and presumably other entities such as users and taxonomy as well?), which is this issue.
b) (pathauto) generating automatic aliases from token-based patterns, which is (probably) the other issue. #229568: [META] Pathauto in Core

And sun: I apologize, I did not fully understand what you were doing until I read this issue (which I was not aware of)... You had marked the "automatic" part as being "later", though, and it seemed like you weren't going to commit to getting this done before feature freeze, which would have left us without Pathauto in Core. Which would be too bad.

A few thoughts on the UI you were working on for the other issue (#229568-36: [META] Pathauto in Core), and how we can divide and conquer this:
- Our usability people thought "Pretty URLs" was a better name than "permalinks" or "aliases"
- I think the separation of prefix and pattern is very confusing (haven't run that by the usability crew, but that's my opinion). So how about if the part where you make this a field does not include the prefix ability, and we work on the prefix/pattern stuff on the other issue?
- I think we can adapt the ideas from #229568-30: [META] Pathauto in Core to the field instance settings vs. content type settings pretty easily. But it would still be helpful to be able to set a default for all bundles of an entity -- our usability team really liked the ability to have three choices: use the default pattern, use a custom pattern, or turn it off.
- I guess with the field design, if you don't want URL aliases, you just remove the field from that bundle, right?

No UI to review :)

Also not convinced we should pull this out of the Vertical tab.

StatusFileSize
new2.74 KB

I think the UI for this issue is basically:
- Add the "Pretty URL" field to any entity. The widget is called "Pretty URL entry".
- No field settings, except you could probably make it Required.
- When editing, it looks the same as what you get now from core Path in D7, except it's a field so it's not in the vertical tabs (just a text box for entering the alias, with a site URL prefix so you understand what part of the URL you are entering, and probably labeled "Pretty URL entry"). The prefix would be either "http://example.com/" or "http://example.com/~q=" depending on clean URLs being enabled or not.
Sorry, I am not fluent in any UI prototyping software, but here is a rough mockup:
basic path widget mockup

Then on #229568: [META] Pathauto in Core, the function of Pathauto is now just to provide a second widget, called "Automatic pretty URL", which I'll discuss there.

I really like the prefix idea and having the user change only the suffix part as part of sun's proposal.

Adding some screenshots from over there:

---
Posted by @sun here: http://drupal.org/node/229568#comment-6684692

  1. Add the field to any entity:
    pathauto-36-manage-fields.png

  2. Edit the path field settings:
    pathauto-36-field-settings.png

  3. Later: Enable automatic aliases:
    pathauto-36-field-settings-auto.png

  4. Add or edit an alias-field-enabled entity:
    pathauto-36-entity-add-edit.png

---

I really love this idea.

I agree that most URL patterns that I have ever defined for client sites start with a prefix. But I find the UI proposed in #12 to be very confusing in how it splits the prefix from the pattern. It is not clear to me from looking at the screen shots how all of the pieces of "URL alias", "URL pattern", "prefix", "automatically create", and "default alias" fit together.

And some more minor UI nitpicks:
- we don't use "node" in UI text in Drupal, since d7
- there's a mix of "alias", "URL", "pattern", and "permalink" terminology.

Please note the following:

  1. I'm still primarily busy with the backend/code side as well as tests for the path alias field implementation to ensure that it works correctly.

  2. The non-customizable prefix has been architecturally designed to be optional. Thus, if you don't want one, just simply don't enter one.

    An existing, configured prefix can be removed later on. The field widget is also able to deal with the situation in which a configured prefix did not exist previously and was only added later. It dynamically adapts itself.

  3. The UX and UI definitely needs polishing. The current state only proves the concept. For now, I simply copied Wordpress' UI, so as to be able to focus on the code.

    I'm very happy to rename strings, reword stuff, rethink the field settings and field widget.
    (That said, this part of Wordpress' UI is actually done very well, and is immediately and intuitively understood by content authors.)

  4. There wasn't a mass-configuration page for path aliases before, so I will defer that feature request to a separate issue.

    I guess people are thinking of something along the lines of WP's global permalink configuration. However, we probably want such a UI to be deeply integrated into the new Blocks/Layouts/Page-manager site building workflow, instead of a random configuration page that exposes all field instances of all entity types. In any case, I consider this overview out of scope for this issue.

I'm sorry, I apparently wasn't clear... I still have major problems with the concept of the prefix:

a) Fundamentally, I do not think that it makes sense to have a prefix as a separate thing from a pattern. If the automatic/pattern functionality exists, it's very confusing to have two boxes to fill in to say "I want all nodes to have aliases like article/[title]" or worse yet, "I want everything to have an alias like archive/[year]/type/[title]".

b) I also think the "prefix" concept (at least as shown in the proposed UI) is confusing by itself, even if there is not any automatic/pattern capability. What happens if the admin has defined a prefix and the content editor has not entered anything in the box -- what URL alias, if any, is generated? What happens to existing node URLs if the admin goes by later and changes the prefix setting? From a developer perspective, does the field store what the content editor enters (the part after the prefix) or the whole alias? I also think that having the prefix setting in the field settings generates an expectation that some kind of pattern will be used to generate an automatic alias, since this is what happens in other CMSs.

So for me, the whole "prefix" thing is just confusing...

As an unrelated note, I'm totally agreed with #14 item 4: with aliases as fields, we should not have a mass-configuration page for aliases. The previous UI suggestions for Pathauto mass config from the other issue will not apply in this case.

Anyway, my suggestion would be to drop the "prefix" idea entirely, and just have a simple UI for entering an alias (see rough mockup in #11). And yes, hopefully you can get the back end working! And yes I know that the UI strings can be changed later. But whether or not there is a prefix concept seems like something that needs to be thought about now.

#15: I tried out Wordpress a little and this makes a lot of sense to me:

Think of it like:

[year]/article/[user-blob]

And user-blob is what the user can _save_ for each field.

Only that here the user-blob is the suffix, always and such implicit instead of explicit.

That way:

Content Admin can say:

All our blog posts start like:

[year]/[category]

And have a default of

[title] as "file name", but the user can change the "file name".

Such writing a blog post like "My great life", I can only change the url from:

2012/posts-that-i-like/my-great-life

to

2012/posts-that-i-like/my-really-great-life-some-seo-keywords-here

but not to:

2013/posts-that-i-do-not-like/my-really-great-life-in-the-wrong-category

And for me that makes total sense, even though it is very different from the drupal concept of a complete path alias.

But _this_ is IMHO made for content editors and probably really one thing that wordpress does really well.

Also see:

http://drupal.org/node/229568#comment-6677880

for an image of the wordpress UI.

I hope this clarifies it a little :-).

RE #16 - what you and WordPress mean by a "prefix" vs. "blob" (which wordpress at least used to call the "url slug", by the way) does not correspond to what this issue is trying to do, which is a completely static prefix with (as I understand it) no tokens in the prefix, and (as I understand it -- again the UI proposed here is very confusing to me so I might be misunderstanding), the token-based patterns would come after the "prefix".

What you're talking about is basically giving the user the ability to override just the [node:title] (or user:name or taxonomy:term) portion of the pattern, I think, and making sure that piece is the last token. Which could be a fine idea but I am not sure it is practical for Drupal if we want to use tokens for the pattern (I *hope* we would not define a similar system that is kind of "token-like"!!!!) and allow arbitrary tokens in the patterns and an arbitrary order.

Tokens would of course be supported in both the prefix and the user-customizable part.

In other places in Drupal, the prefix will mean language code prefix, such as example.com/fr/node/123. Like jhodgdon, I think this will confuse matters.

Well, as said, better naming suggestions are welcome.

That said, the language code prefix you're referring to is actually a direct equivalent, not something different — The language path prefix is also a site/context-wide path prefix that is configured for one context and automatically prepended to the final path/alias.

So that would actually be an argument for labeling this with "prefix", not against. ;) But hey, I'm not married to the labels, only the content authoring experience. :)

Status:Active» Needs review
StatusFileSize
new42.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path.field_.21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

FWIW, here's the latest code, if anyone's interested in testing it.

Also switched back to DrupalUnitTestBase, incorporating #1774388-88: Unit Tests Remastered™, so I can properly work on the tests without having to wait minutes for each run. Those changes are of course to be ignored in this patch, and I'll remove them once this is ready (or when the follow-up patch over there has landed). For now, that's just to keep things sane.

Fwiw this can still live in the vertical tabs if the widget form is wrapped in a fieldset and has the group set, we were able to get that working over in #731724: Convert comment settings into a field to make them work with CMI and non-node entities where there is a similar concept (eg take something that used to form_alter the node forms and put it into the field api), see the patch over there for an example (would love some reviews too)

Status:Needs review» Needs work
Issue tags:-Usability, -Killer End-User Features, -token, -pathauto, -Platform Initiative

The last submitted patch, path.field_.21.patch, failed testing.

Status:Needs work» Needs review

#21: path.field_.21.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +Killer End-User Features, +token, +pathauto, +Platform Initiative

The last submitted patch, path.field_.21.patch, failed testing.

Please don't make it difficult for end users and have two different things: prefix and pattern. Please just use the well-established concept of just using a pattern.

I am still in favor of the simple UI in #10 for the case where there are no tokens/pathauto:
ui mockup

And then if we have pathauto, we would choose a different widget on the Field setup, and it would look something like:
second UI mockup

[as usual, excuse my rough UI sketches please!]

I don't understand the need to differentiate between a prefix and a pattern. From what I can see, no real rationale has been given for it either. Why a prefix?

I also took a look at the patch in #21: It seems to have some unrelated stuff in it, like changes to schema.inc and updateModules and some kind of a null widget? I don't understand why that is there, so I'll wait until we have a clean patch to look at it more carefully.

It also seems to be very tied up in the idea of the prefix and the alias entered, so that it would be very difficult to use tokens in it by merely substituting a different widget. So I think as architected, it will not work with PathAuto as a widget.

Status:Needs work» Needs review
StatusFileSize
new51.42 KB
FAILED: [[SimpleTest]]: [MySQL] 48,160 pass(es), 40 fail(s), and 5 exception(s).
[ View ]

Re-rolled and multi-patch including soon-to-be-commited unit tests remastered.

See PATCH 2/2 for the real patch for this here.

Use temporary branch and:

git am file.patch

to apply and test.

Status:Needs review» Needs work

The last submitted patch, core--pathauto--1751210--30.diff, failed testing.

#28 Prefix helps admin control the url while still gives contributors the flexibility. Think like:
- you don't want French content to have an en/ prefix nor the contributor bother to prefix it with fr/
- you want contributors to customize 'news' url, but it must begin with year/month/day/
- you want contributors to fully customize url of 'article' content (in this case specify an empty prefix for 'article' node type)

I think all of them are very nice to have in core, while still keeps the change minimal.

Couldn't we still accomplish this in a single text field if we just provide a string to Denmark which part of the path is editable by content authors? Like bracket the editable part between "[[" and "]]", or something like that. This could also allow the flexibility of making the middle part of the path editable, while holding the beginning and end parts fixed.

Seems like the prefix is not part of the original task, which is to make the existing functionality a field as opposed to make the existing functionality a field + add new functionality.

Seems like the main thing that could potentially hold this patch up is discussion around prefix, which could have a separate issue of its own.

Having said that, I can definitely see the prefix use case.

I have had numerous clients that have overridden the entire path when I would have preferred they only be overriding the latter part.
Even well trained users have the free will to do things they shouldn't if they are given the opportunity.

Yes, I understand the idea of the prefix now, and I can see its usefulness. Thanks jcisio for explaining. As a sitebuilder I want to control the whole of the path that gets generated and allow only one or a few segments of it to be generated by content creators. Seems quite useful indeed.

Agreed with rooby that this is not part of the original task though and at this point counter-productive to try and incorporate. I opened [1844438] as a follow-up, lets focus back on the original scope of things in here. Thanks all.

Title:Add path aliases as a default field on all content typesConvert URL alias form element into a field and field widget
Status:Needs work» Needs review
StatusFileSize
new16.22 KB
new22.73 KB
FAILED: [[SimpleTest]]: [MySQL] 48,147 pass(es), 41 fail(s), and 5 exception(s).
[ View ]
  1. Merged HEAD.
  2. Removed non-customizable path prefix functionality.

Status:Needs review» Needs work

The last submitted patch, path.field_.37.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.29 KB
new44.64 KB
FAILED: [[SimpleTest]]: [MySQL] 48,305 pass(es), 33 fail(s), and 2 exception(s).
[ View ]

Major, and possibly final changes:

  1. Merged HEAD.
  2. Fixed path_path_delete() does not disable entity access checks.
  3. Removed Article node type dependency from Path module tests.
  4. Added helper functions to create default path field and field instance.
  5. Reverted #1739986: Fix fallback in drupal_get_hash_salt(), move it to bootstrap.inc, use instead of $GLOBALS['drupal_hash_salt']
  6. Fixed existing path in field cannot be deleted.
  7. Updated PathTaxonomyTermTest.
  8. Updated PathAliasTest.
  9. Updated PathLanguageTest. Required to convert it to Entity Translation.
  10. Updated PathLanguageUiTest.

All existing and new Path module tests should come back green.

+++ b/core/modules/path/path.moduleundefined
@@ -39,6 +37,127 @@ function path_help($path, $arg) {
+/**
+ * Adds the default path field to an entity bundle.
+ *
+ * @param string $entity_type
+ *   The name of the entity type to add the field to.
+ * @param string $bundle
+ *   The name of the bundle to add the field to.
+ *
+ * @return array
+ *   The path field instance.
+ */
+function path_add_default_field_instance($entity_type, $bundle, $label = 'Permalink') {
+  $field = path_add_default_field();

Looks like @param directive for $label is missing.

Status:Needs review» Needs work

The last submitted patch, path.field_.39.patch, failed testing.

Eh?

Status:Needs work» Needs review
Issue tags:-Usability, -Killer End-User Features, -token, -pathauto, -Platform Initiative

#39: path.field_.39.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +Killer End-User Features, +token, +pathauto, +Platform Initiative

The last submitted patch, path.field_.39.patch, failed testing.

StatusFileSize
new26.57 KB
new22.1 KB

@sun I think tests fails because the path module cannot be disabled due it say it has fields that are being used.

pathfield.png

Sadly there is no field in the UI related to the path module.

fieldlist.png

I hope this help to figure out the problem

Status:Needs work» Needs review

@dagmar: Wow, thank you! :)

Guess I understand why Node module does not install its default 'body' field in node_install() now... ;)

Will adjust for that tomorrow, should be an easy tweak. Ignoring those failing tests, it's time for substantial architectural reviews.

@sun, @dagmar I had similar issues over in #731724: Convert comment settings into a field to make them work with CMI and non-node entities and got around them using field_associate_fields in the hook_uninstall, before calling field_delete_field().

See eg removing the comment field from forum nodes in forum_uninstall() -http://drupalcode.org/sandbox/larowlan/1790736.git/blob/refs/heads/comme...

Had a read over this and it looks very similar to the approach used in #731724: Convert comment settings into a field to make them work with CMI and non-node entities, only question I have is with the use of und in tests, some of core's tests use $langcode = LANGUAGE_NOT_SPECIFIED then throughout the test, create their $edit variables like so

<?php
$edit
= array(
 
"field_foo[$langcode][0][value]" => 'some value'
);
?>

Is there a convention for this?

What do we do with entity types that are fieldable but don't have a uri callback? Can we prevent the user from adding the field to those entity types? Should we? Struggling to think of an example in contrib that has fieldable entities without a uri anyway.

Tested and works as advertised, couple of issues
In D7 we had help text here to tell the user what the field is for, I can't add this help text using the field ui (see screenshots). Should we have a sensible default for the help text, not disimilar to what the old form used?

Screen Shot 2012-11-21 at 5.08.11 PM.pngScreen Shot 2012-11-21 at 5.08.37 PM.png

StatusFileSize
new35.76 KB

had a look into whether field_associate_field is going to help here but its not.
In #731724: Convert comment settings into a field to make them work with CMI and non-node entities the problem was forum module was preventing comment from uninstalling.
So in forum_uninstall we could deal with the field removal.
The issue here is we can't deal with the removal at hook_uninstall() in our own module (path_uninstall()), the field has to be gone beforehand.
This leaves as I see it two options

  • Remove the path_install() that sets up the field, I couldn't see that field in the ui anyway (see screenshot)
  • Remove the path field in hook_disable() however this violates our policy on disabling a module have non-destructive changes.

Screen Shot 2012-11-21 at 6.07.20 PM.png

Confirming that path_add_default_field() doesn't mean the field shows up in the UI, as field_ui_existing_field_options runs off field_info_instances() and not field_info_fields(), damn

Thanks for reviewing, @larowlan!

re: #48: "Default instance properties"

I actually added a couple of @todos with regard to this in the PathWidget class — Field API does not have any notion for providing a "default label", nor a "default description", etc.pp. for field instances.

Consequently, the site admin/builder can add the URL alias field to various entities, but he/she always has to manually re-specify the label for each instance. Thus, you can have a "Permalink" on nodes, a "URL alias" on taxonomy terms, and a dozen of other variations across the system, because all of them need to be set up manually.

Compared to D7, that could be considered as a "UX regression", since the URL alias widget was always exposed in the same way on all entity forms.

Due to that, the current PathWidget in the patch actually ignores the label that has been configured for the field instance, and instead, always outputs "Permalink" as label.

I'm not sure what to do on that front. Hard-coding "Permalink" (or whatever else; the actual label does not matter) does not feel right to me, but at the same time, Field API does not provide a facility to provide and ensure default properties for all instances of a field.

I'm tempted to remove the hard-coded label and just simply accept that this aspect needs to be addressed elsewhere in Field API, independently from this issue.

I'll also check what's going on with the help text — most probably that just needs to be assigned as #description.

re: #49: Default 'path' field.

Yeah, I'm planning to remove the code in path_install() entirely and will look some more into Node module to see what exactly it is doing for its default 'body' field, and will try to replicate that for the default 'path' field.

Just a thought on #51... To me, "Permalink" is a term that is specific to blogs. I have never once encountered a web site owner that I was setting up a generic Drupal site for who used that term. So I would definitely avoid hard-wiring it into the UI of this! Obviously, "Path alias" is not a very intuitive term for a non-Drupal-expert user either, in my experience with clients, nor is "URL alias".

At BADCamp when we had a brainstorming/design session thinking about Path Auto, we came up with the idea of "Pretty URL" for a more generic term, which I think would be a good user-facing name for the field and widget actually. Then the Path Auto widget could be called "Automatic pretty URL", which is wordy but at least clear?

In any case, I think that letting users set their own labels and help text is fine, and that the change from calling it "path alias" or "URL alias" (which I'm always having to either form alter or explain to my clients) to letting it be defined by the user is a GOOD thing. :)

+1 for flexible title.

Should we have a new class extending FieldItemBase for the new entity field api here?

+++ b/core/includes/bootstrap.incundefined
@@ -1995,19 +1995,6 @@ function drupal_hash_base64($data) {
- * Gets a salt useful for hardening against SQL injection.
- *
- * @return
- *   A salt based on information in settings.php, not in the database.
- */
-function drupal_get_hash_salt() {
-  global $drupal_hash_salt;
-  // If the $drupal_hash_salt variable is empty, a hash of the serialized
-  // database credentials is used as a fallback salt.
-  return empty($drupal_hash_salt) ? hash('sha256', serialize(Database::getConnectionInfo('default'))) : $drupal_hash_salt;
-}

This seems out of scope for this issue?

+++ b/core/misc/vertical-tabs.cssundefined
@@ -62,7 +62,7 @@ fieldset.vertical-tabs-pane > legend {
.vertical-tabs .form-type-textfield input {
-  width: 100%;

same, there is no change to vertical tabs here, other than removing the path tab from node forms, does this belong in another issue?

Quite heavy merge conflicts with HEAD, which I was able to resolve, but due to #1269742-97: Make path lookup code into a pluggable class, I actually hope for a (quick) rollback of the change that caused the conflicts.

StatusFileSize
new41.4 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
  1. Merged HEAD (again).
  2. Updated for #1269742: Make path lookup code into a pluggable class

Interdiff is not possible.

Will work on the label + help + all other review issues now.

StatusFileSize
new4.88 KB
new42.14 KB
FAILED: [[SimpleTest]]: [MySQL] 48,807 pass(es), 9 fail(s), and 5,572 exception(s).
[ View ]
  1. Moved default path field/instance configuration into standard.install.
  2. Fixed configured field instance label and help text does not appear in form widget.
  3. Ensure the site URL being output as #field_prefix always ends with a slash.

StatusFileSize
new5.36 KB
new42.82 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
  1. Fixed node $type is an array in standard.install.
  2. Fixed path_field_update() for new Path\AliasManager.

Status:Needs review» Needs work

The last submitted patch, path.field_.57.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new563 bytes
new42.82 KB
FAILED: [[SimpleTest]]: [MySQL] 48,722 pass(es), 3 fail(s), and 20 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, path.field_.59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new861 bytes
new43.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Fixed Field UI DisplayOverview wrongly assumes a default formatter to exist.

Created #1849724: Avoid critical Field API limitations in the D8 Field API sandbox project to make Field API maintainers aware of these issues.

The PathFieldCRUDTest test failures are expected and can be ignored for now. They will automatically pass as soon as #1848072: Path alias manager hardcodes keyvalue.database instead of using the generic factory has been committed.

Status:Needs review» Needs work
Issue tags:-Usability, -Killer End-User Features, -token, -pathauto, -Platform Initiative

The last submitted patch, path.field_.61.patch, failed testing.

Status:Needs work» Needs review

#61: path.field_.61.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +Killer End-User Features, +token, +pathauto, +Platform Initiative

The last submitted patch, path.field_.61.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
new43.67 KB
FAILED: [[SimpleTest]]: [MySQL] 48,756 pass(es), 3 fail(s), and 6 exception(s).
[ View ]

Sorry, I forgot to merge @dagmar's fix from #59.

  1. Fixed $type in standard.install is an object.
  2. Renamed $prefix into $field_prefix to account for follow-up issue.

Status:Needs review» Needs work

The last submitted patch, path.field_.65.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.39 KB
new46.25 KB
FAILED: [[SimpleTest]]: [MySQL] 48,734 pass(es), 2 fail(s), and 9 exception(s).
[ View ]
  1. Incorporated essential fixes from #1848072: Path alias manager hardcodes keyvalue.database instead of using the generic factory to get a green light here.
  2. Fixed fatal error in tests caused by bogus merge.
  3. Fixed Field UI DisplayOverview wrongly assumes that each field would have display settings.

Status:Needs review» Needs work

The last submitted patch, path.field_.67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.2 KB
new47.7 KB
PASSED: [[SimpleTest]]: [MySQL] 48,818 pass(es).
[ View ]
  1. Fully incorporated #1848072: Path alias manager hardcodes keyvalue.database instead of using the generic factory
  2. Fixed bogus variable names are merge conflict.

Alright. I consider this patch as done and ready to be committed.

Since we still need to get #1844438: Allow part of the generated path to be set as a fixed prefix done as a usability improvement for D8, which seems to be bound to feature freeze, I'd love if we could quickly move forward here.

Additionally, I think the entire work for #229568: [META] Pathauto in Core is blocked on this conversion, too.

sun: I'd be happy to review this patch, but when I start reading it, it starts with a bunch of stuff that I don't understand, which makes me say "huh, how's this related to making the path alias into a field?". Like this stuff for instance:

-use Drupal\Core\KeyValueStore\KeyValueDatabaseFactory;
+use Drupal\Core\KeyValueStore\KeyValueFactory;

So as an aid to the reviewer, would you consider providing an overview of all of what the patch does, and why?

Please ignore those KeyValue related hunks — I merged #1848072: Path alias manager hardcodes keyvalue.database instead of using the generic factory into this patch, only in order to get a green light testbot confirmation here.

Hm. OK, what about this hunk?

-          'defaultFormatter' => $field_types[$field['type']]['default_formatter'],
+          'defaultFormatter' => isset($field_types[$field['type']]['default_formatter']) ? $field_types[$field['type']]['default_formatter'] : 'hidden',

Again, a road map to what this patch does would be helpful. :)

Issue summary:View changes

Updated issue summary.

Ah, heh, sorry, yeah... OK, I've updated the issue summary and clarified the situation.

In essence, just focus on the changes in /core/modules/path/* only. :)

StatusFileSize
new43.66 KB
PASSED: [[SimpleTest]]: [MySQL] 48,837 pass(es).
[ View ]

Merged HEAD. (aforementioned DUTB fix landed)

@sun - Are there supposed to be blank lines after an opening class brace and before the class declaration closing brace? This patch has it both ways.

@Lars Toomre: Yes, it's in the standards, but not consistently followed yet. But that's a detail no one should stress on. We can go through all classes in core at once before release to adjust them.

Lars #46: we don't have a standard on this. Not a big deal.

So, I took a look at the code in this patch (just the parts in modules/path). I found a few things that concerned me (so far, I haven't tried out the module -- was just looking at the code):

a) in the Widget class's validatePath() method:

+      $query->addExpression('1');

What does this do? seems wrong/extraneous.

b) In PathLanguageTest... in the setUp() function, I would be more comfortable if the "set translatable" parts were done through the UI like the rest of the setup, rather than using field_update_field(). Is that even possible? And is this section of the patch merging in a bunch of unrelated stuff from another issue regarding field/entity translation? I didn't see that mentioned in the review notes.

c) In PathLanguageTest -- why change the local variable $english_node to just $node? That seems to make it less clear than it was before. I would leave that alone, and some of the later changes from $french_node to just $node are also less clear than they were before.

d) In PathTaxonomyTermTest -- in the setup function, I noticed this line:

+    path_add_default_field_instance('taxonomy_term', 'tags');

and similarly in PathTestBase:
+      path_add_default_field_instance('node', 'page');

That implies a major UI regression... are we OK with that? At least add it to the issue summary (the issue summary is missing the API and UI sections currently -- they need to be added). Which is to say: previously (I think), both nodes and taxonomies had path alias capability by default. Now if you create a content type or a vocabulary, you have to add the Path field yourself.

f) In path_field_schema(), the pid field description should say it's a foreign key to the {url_alias} table, rather than saying 'A unique path alias identifier.' (which would imply that it's just some unique ID rather than particularly the one from {url_alias}.pid). I see in the lines below that there's a foreign key defined, but it should be part of the description. As an example, {node}.vid has this description:

        'description' => 'The current {node_revision}.vid version identifier.',

Do something similar.

g) It seems to me that path_field_is_empty() should return TRUE if the 'value' part is empty (independent of the 'pid')? That is what the user enters.

h) This code comment in path_field_insert() is totally wrong:

+      // path_save() populates $item['pid'].
+      $fields = drupal_container()->get('path.crud')->save($item['source'], $item['alias'], $item['langcode']);
+      $item['pid'] = $fields['pid'];

i) I'm concerned in path_field_update() -- will this produce PHP warnings if the field is empty (can it be NULL or is it always a string)? Is this condition tested in one of the tests? (I think we should be testing: 1. creating a node in the UI with a path field left empty. 2. Then updating this node so the path field is still empty. 3. Then updating to give the path alias a value. 4. Then updating again to empty out the value. Make sure there are no PHP notices in any of these operations, and that the alias works in 3 and is then removed in 4. I don't think the tests cover all of this?)

+    $item['value'] = trim($item['value']);
+    // Delete old alias if it was erased.
+    if ($item['value'] === '') {

j) Another concern with path_field_update() -- it looks like it will override a manually-added URL alias (one that was added in the generic Path admin page). Is that desirable? I think it will override only the first alias found... maybe? Did the path module do that before, or is it a UI change? And what happens if someone uses the path field and also adds an alias on the Path admin page?

k) in path_field_delete(), I don't think the & is necessary here:

+  foreach ($items as &$item) {

l) I am slightly concerned in path_add_default_field() with assuming that no one else could possibly have added a field with machine name 'path'? Do we make that kind of assumption elsewhere in Core with setting up default fields?

m) In path_add_default_field_instance(), shouldn't the default field label of 'Permalink' be translated? And as I've stated before, I don't think "Permalink" is a good default label anyway... but we can change that later I guess.

n) Does this field allow multiple values? Should it?

Status:Needs review» Needs work

Well, no more reviews yet, I guess I should set this back to "needs work" on the basis of #78?

Just reviewing this quickly, I see multiple labels for the thing - I can understand the final label might be called different. But it seems weird the end-user has to figure out, each step from Path, URL Alias, to Permalink what the thing is called - that looks like a serious issue? Can't we go with one label, like "URL" - we know users don't know what Path means or Permalinks, perhaps URL is the lucky charm :P?

Also I would really like us to keep this in the VT area, I know Wordpress is awesome but I feel with our audience its far less common for this setting to be changed.

FWIW, it would be a first, but absolutely no problem at all for standard profile to create a path field on node types and then form_alter the widget form back into a vertical tab.

That wouldn't help with newly created content types, etc. but wanted to point that out nonetheless.

Thinking about, if people choose to *re-use* the existing path field, we could actually make the above work for arbitrary node types, whether admin-created or not.

Of course in a perfect world, Field UI would allow to group fields in vertical tabs anyway, but that might be a long shot. (~ fieldgroup.module in core)

If the profile-created types behave differently from user-added types, I am about 99% against the idea in #81. But this doesn't just apply to nodes anyway -- taxonomy terms, users, and other entities can have URL fields. So any solution for moving the field into the vertical tabs should be applied to all entities. To do otherwise will just cause confusion IMO.

Status:Needs work» Needs review
StatusFileSize
new4.74 KB
new44.72 KB
PASSED: [[SimpleTest]]: [MySQL] 49,307 pass(es).
[ View ]

@jhodgdon:

+ $query->addExpression('1');
What does this do? seems wrong/extraneous.

It performs a SELECT 1 to not actually query any values, since the entire query's purpose is to check for an existing value only. That's nothing new, but rather common pattern throughout core.

@see db_query_range()

b) In PathLanguageTest... in the setUp() function, I would be more comfortable if the "set translatable" parts were done through the UI like the rest of the setup, rather than using field_update_field(). Is that even possible? And is this section of the patch merging in a bunch of unrelated stuff from another issue regarding field/entity translation? I didn't see that mentioned in the review notes.

Marking as field as translatable is not within the scope of Path module or the path field functionality. That is to be tested by translation_entity module, and only there. Tests of other modules should never duplicate such operations, but always use the API operations instead.

c) In PathLanguageTest -- why change the local variable $english_node to just $node? That seems to make it less clear than it was before. I would leave that alone, and some of the later changes from $french_node to just $node are also less clear than they were before.

The test is converted from the old translation module to the new translation_entity module. This is required, since path is turned into a field, and is thus based on the field language instead of the entity language. The old translation module does not support fields. Without converting test to translation_entity module, the entire test would have to be deleted.

With translation_entity module (translatable fields), there are no longer two English/French nodes, but there is a single $node only, whose fields have language negotiation baked in. Welcome to the New World Order Of Translatable Fields™ :)

d) In PathTaxonomyTermTest -- in the setup function, I noticed this line:
+ path_add_default_field_instance('taxonomy_term', 'tags');
...
+ path_add_default_field_instance('node', 'page');

That implies a major UI regression... are we OK with that? At least add it to the issue summary (the issue summary is missing the API and UI sections currently -- they need to be added). Which is to say: previously (I think), both nodes and taxonomies had path alias capability by default. Now if you create a content type or a vocabulary, you have to add the Path field yourself.

This is explicitly stated in the issue summary already — URL alias support can be added to any entity, and also removed from any entity.

It is not possible to automate this with the current Entity/Field API (for all content entity types) yet, because we do not have a proper Entity Bundle API in core yet. The patch takes the identical approach as Node module does for its Body field.

(To prevent confusion, Node module is able to automate the Body field, because it controls the entity type and thus it is able to plug that operation at the right time into the node type bundle creation.)

Allowing field type modules to do this is a separate issue, which we should try to address for D8 to simplify the site builder interaction here.

[There was no e)]

f) path_field_schema() is addressed.

g) It seems to me that path_field_is_empty() should return TRUE if the 'value' part is empty (independent of the 'pid')? That is what the user enters.

I had a similar thought and that was actually how the code looked before. However, the path field is essentially a reference field, which means that a non-empty reference ID always takes precedence. Apparently, Field API first invokes the _field_is_empty() callback to determine whether any field value needs to be deleted. Thus, if the 'value' for an existing alias has been emptied, then the 'value' is empty but there was a 'pid' before and the referenced path alias ID still needs to be deleted. Therefore, the primary condition has to be 'pid' for existing values, and only new field values won't have that, but instead, a non-empty 'value' instead.

Added a code comment to clarify this.

h) Fixed.

i) I'm concerned in path_field_update() -- will this produce PHP warnings if the field is empty

Field API ensures that the defined schema columns of a field type exist in each item. Nothing to test here.

j) Another concern with path_field_update() -- it looks like it will override a manually-added URL alias (one that was added in the generic Path admin page). Is that desirable? I think it will override only the first alias found... maybe? Did the path module do that before, or is it a UI change? And what happens if someone uses the path field and also adds an alias on the Path admin page?

Yes, path module did this before. If there is an existing path alias ID and if it is identical, then that existing path alias is updated.

This is essentially also the reason for why I did not implement path_field_load() to populate (+ cache) the path field values for each entity that is loaded, because it means that this cached information can get outdated too easily, which in turn would mean a potential ID mismatch, which in turn would mean that submitting a form could create duplicate URL aliases.

Removed the corresponding todo comment, thanks.

k) There's absolutely nothing wrong with array value references, but I've removed it nevertheless.

l) I am slightly concerned in path_add_default_field() with assuming that no one else could possibly have added a field with machine name 'path'? Do we make that kind of assumption elsewhere in Core with setting up default fields?

This is why all custom/user-defined fields are prefixed with 'field_'. Module-defined fields do not have that prefix. Beyond that, there is no additional protection in the field system for potential clashes. However, this is exactly what we're doing elsewhere in core, so that's off-topic for this issue.

m) In path_add_default_field_instance(), shouldn't the default field label of 'Permalink' be translated?

Nope, module-defined field labels are English by default. This code is essentially copied from Node module.

n) Does this field allow multiple values? Should it?

That's a separate feature request, which is not to be discussed here. :)

All other issues raised in later comments have been addressed as well. I strongly disagree with some of them, but I rather want to get this feature into core instead of debating over things that can be re-evaluated and adjusted in follow-up issues to begin with. ;)

  1. Fixed foreign key description in path_field_schema().
  2. Clarified condition in path_field_is_empty().
  3. Fixed comment and variable in path_field_insert().
  4. Removed obsolete todo about loading and caching field values outside of field widget.
  5. Removed array value reference from path_field_delete().
  6. Added dynamic vertical tabs integration, if existent.
  7. Replaced all instances of "Permalink" with "URl alias".

#83: path.field_.83.patch queued for re-testing.

I am not at a machine currently that has git. Hence, I am unable to re-roll a patch that would address the following observations about the patch in #83:

+++ b/core/modules/path/lib/Drupal/path/Tests/PathFieldCRUDTest.phpundefined
@@ -0,0 +1,144 @@
+    // Create a path field for the node type.
+    $this->field_name = drupal_strtolower($this->randomName());
+    $this->langcode = LANGUAGE_NOT_SPECIFIED;
+    $this->field = array(
+      'field_name' => $this->field_name,
+      'type' => 'path',
+    );
+    field_create_field($this->field);
+    $this->instance = array(
+      'field_name' => $this->field_name,

Shouldn't any members of a class be documented as well at the top of the class? Also, my understanding is that a member name like 'field_name' should be something like 'fieldName' (camelCase).

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.phpundefined
@@ -31,18 +31,26 @@ function setUp() {
+    $this->web_user = $this->drupalCreateUser(array('edit any page content', 'create page content', 'administer url aliases', 'create url aliases', 'administer languages', 'translate any entity', 'access administration pages', 'administer content types'));
     $this->drupalLogin($this->web_user);

Same comment about camelCase members. Also for readability, it would make sense to break each of these permissions out into a separate line.

+++ b/core/modules/path/path.moduleundefined
@@ -39,6 +37,151 @@ function path_help($path, $arg) {
+ * @param string $entity_type
+ *   The name of the entity type to add the field to.
+ * @param string $bundle
+ *   The name of the bundle to add the field to.
+ *
+ * @return array
+ *   The path field instance.
+ */
+function path_add_default_field_instance($entity_type, $bundle, $label = 'URL alias') {
+  $field = path_add_default_field();

Missing @param directive for $label variable.

StatusFileSize
new531 bytes
new45.22 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/path/lib/Drupal/path/Plugin/field/widget/PathWidget.php.
[ View ]

Regarding 1) and 2):
Test classes do not necessarily declare the properties they are using, which is perfectly fine, since test classes are self-contained and only executed on their own. The properties $this->field_name, $this->web_user, $this->admin_user are common standard property names we are using in tests. Therefore, there's nothing wrong with these properties.

Addressed the 3rd issue:

Fixed missing @param for $label function argument.

Any chance we can move forward here?

Status:Needs review» Needs work

The last submitted patch, path.field_.86.patch, failed testing.

Sorry sun, I wanted to review your latest patches but I have been really busy with work and haven't had much Drupal volunteer time lately. I have a couple of transliteration follow-up patches that could use reviews too...

Status:Needs work» Needs review
StatusFileSize
new1014 bytes
new44.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path.field_.89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed bogus merge conflict resolution.

Sorry, I'm still really busy and won't have time to do any major patch reviews until January or so. If you can recruit someone else to review this patch, that would be good.

#89: path.field_.89.patch queued for re-testing.

I really don't think there's anything more to complain about this patch. It's a massive step in the right direction.

I just reviewed the changes once more myself, after forgetting about it for a couple of days.

Yes, there are a few implications and consequences with this patch; mainly revolving around Field API limitations; i.e., there is no mechanism for a field to "auto-attach" itself to new entity bundles, and there is currently also no way for a field to help to ensure sane default properties for new instances of itself (e.g., to achieve a consistent field label across instances, etc).

However, all of that is unrelated to this issue and patch, and needs to be figured out in dedicated Field API issues instead. This change merely helps to expose these limitations and increase awareness for them.

The change itself is a killer feature though: This patch enables you to have a URL alias widget on every entity type/bundle where you want to have it! And: Don't want URL alias functionality for a particular one? Just remove the damn field instance. :)

All the one-off module integrations with nodes and taxonomy terms are a thing of the past. That's exactly in line with the general spirit and direction we're moving towards.

It is also the ultimate answer for the epic debate on "Does module X integrate with module Y? Or does module Y integrate with module X?", which previously existed; i.e., does Path module implement hook_form_node_form_alter(), or does Node module check module_exists('path') to conditionally add a URL alias widget to the form? That question is entirely obsolete with this change. We very likely want to perform the exact same change for the Menu link widget, too.

So, RTBC, anyone? :)

sun: I'm sure I asked this previously, but could you please update the issue summary, paying special attention to the usability and UI changes? They are not mentioned there at all, which makes it difficult for usability people and/or committers to evaluate whether or not they agree with your assessment in #92 that this is a good idea. Just set out the facts: what we lose, what we gain. Thanks!

I don't think this is near RTBC, the UI has been reviewed minimally and as far as I see its still not possible to be placed in the Vertical tabs. Until we resolve this, to me its a regression - since we are exposing functionality into the "content fields area" that is considered meta, and therefor to be placed in Vertical tabs. Our audience is not the Wordpress audience, and the URL often has less significance and/or with pathauto less likely to be configured. It being placed outside VT should be optional, not mandatory.

Re #94: That's not true anymore. The current patch makes the path field automatically insert itself into the vertical tabs on nodes and any other entity types which define a "additional_settings" fieldset. I hope to test this in detail in the coming days and will post some screenshots when I do.

...We very likely want to perform the exact same change for the Menu link widget, too.

Did anybody file an issue for this btw?

Status:Needs review» Needs work
Issue tags:+Usability, +Killer End-User Features, +token, +pathauto, +Platform Initiative

The last submitted patch, path.field_.89.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new44.8 KB
FAILED: [[SimpleTest]]: [MySQL] 49,670 pass(es), 42 fail(s), and 205 exception(s).
[ View ]

Merged HEAD.

Status:Needs review» Needs work

The last submitted patch, path.field_.99.patch, failed testing.

The errors are mostly in path_field_insert() so seem to be related to this patch.

Can we get a new patch? I'd like to devote some time to getting this patch reviewed and ready for commit before it is too late, if possible.

Status:Needs work» Needs review
StatusFileSize
new44.78 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Yep, this was on my plate for this week anyway already, so here's a patch with HEAD merged + resolved conflicts.

I guess the test failures will be similar to the last, so looking into those now.

StatusFileSize
new1.86 KB
new44.79 KB
FAILED: [[SimpleTest]]: [MySQL] 48,673 pass(es), 25 fail(s), and 45 exception(s).
[ View ]

Updated for $entity_type parameter removed from Field API hooks.

Status:Needs review» Needs work

The last submitted patch, path.field_.104.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new45.38 KB
FAILED: [[SimpleTest]]: [MySQL] 48,777 pass(es), 25 fail(s), and 3 exception(s).
[ View ]
  1. Fixed Path::delete() tries to operate on a non-existing URL alias object.
  2. Renamed new path field CRUD test to clarify that it is a unit test.

Hopefully, we're back to green with this.

Status:Needs review» Needs work

The last submitted patch, path.field_.106.patch, failed testing.

I thought I was almost done, but for a couple of hours, I'm loosing my hairs on entity_translation module... #1904880: Translation entity module does not provide any API

Status:Needs work» Needs review
StatusFileSize
new7.19 KB
new48.02 KB
PASSED: [[SimpleTest]]: [MySQL] 48,696 pass(es).
[ View ]

Finally managed to work around the ET API/test-setup problems.

Related @todo: #1899862: WebTestBase::$root_user does not have a uid

  1. Fixed Field API FormatterPluginManager blows up on a field type without any [default] formatter.
  2. Updated PathWidget for node form 'advanced_settings' container renamed to 'advanced'.
  3. Fixed bogus merge conflict resolution in PathTaxonomyTermTest.
  4. Fixed PathLanguageTest.

Technically, this should come back green.

Status:Needs review» Needs work
StatusFileSize
new11.6 KB

I tested this patch today in detail. It doesn't seem to be working. Here is what I did to test:

a) Git pull, apply patch, install Drupal using UI with the "standard" profile. Remain logged in as user 1.

b) Click on Content in the menu, click Add Content button, and choose Basic Page (so I'm on node/add/page).

c) There is a section at the bottom labeled "URL Path Settings" (which I think is a bad label for new users -- it shouldn't have the word "Path" in it, as discussed above. Can it just be "URL Settings"?). The section label is black instead of blue like the others, and clicking it has no effect (there is nothing in it). See screen shot "stdinstall".

d) Tried the same thing with the "Article" content type (node/add/article). Same result as (c).

e) Went to Structure / Content Types / edit for Basic Page / Manage Fields (so I am on admin/structure/types/manage/page/fields). Strangely, the URL Alias field is there (it is definitely not showing up on the add/edit page). Its machine name is "path", which again I think could be confusing for users (maybe the system-added field should have a machine name of "url" instead?), but that is a relatively minor issue.

f) Created a new content type (admin/structure/types/add) called "test". When I clicked "Save and manage fields", it didn't have the URL field. I chose to add the existing "path" field, and I got this warning message when it took me to the edit page (I have all notice-level PHP warnings enabled):
Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->prepareConfiguration() (line 132 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).

g) Went to add content (node/add/test) for this new content type. Same result as (c).

h) Went back to my "test" content type, removed existing path field, and created a new field of type "URL Alias", which I labeled "URL" and it was given machine name "field_url". I got the same PHP notice as in (f) when I got to the edit screen.

i) Repeated (g), same result.

So... In summary, this is not working for me: I cannot see the URL alias field on the node/add page, even though the Manage Fields page says that the field is there.

Status:Needs work» Needs review
StatusFileSize
new2.09 KB
new48.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path.field_.111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

re: #110: We're bitten by two different problems there:

1) #1838114-87: Change node form’s vertical tabs into details elements — I'm not working with the Standard profile (never), so I've never seen this alternate user interface, since it currently requires an obscure checkbox in the Appearance settings to be enabled.

2) #1852104: #type details: Remove #collapsible property — somewhat related to 1), since no vertical-tabs/details processing happened for other elements in that user interface, there was no single details element with a #collapsible TRUE property, which is the trigger for loading collapse.js. The patch over there is RTBC for quite some time already and removes the #collapsible property entirely from core. Attached patch adds it back to the path widget as a stop-gap fix. We need to remember to remove it as soon as that patch has landed.

Additionally, #1899862: WebTestBase::$root_user does not have a uid just landed, so we can remove the stop-gap fix from the test.

  • Fixed PHP notice after creating a field instance.
  • Fixed path field widget details element is not #collapsible.
  • Removed stop-gap fix for $this->root_user->uid.

Status:Needs review» Needs work

The last submitted patch, path.field_.111.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new48.05 KB
PASSED: [[SimpleTest]]: [MySQL] 49,036 pass(es).
[ View ]

Merged HEAD and Removed stop-gap fix for #collapsible.

+++ b/core/lib/Drupal/Core/Path/Path.php
@@ -130,7 +130,9 @@ public function load($conditions) {
-    $path = $this->load($conditions);
+    if (!$path = $this->load($conditions)) {
+      return;

no way to delete alias without path?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php
@@ -85,7 +85,16 @@ public function getInstance(array $options) {
-      $plugin_id = $field_type_definition['default_formatter'];
+      if (!empty($field_type_definition['default_formatter'])) {
+        $plugin_id = $field_type_definition['default_formatter'];
@@ -115,11 +124,14 @@ public function prepareConfiguration($field_type, array $configuration) {
-      $configuration['type'] = $field_type['default_formatter'];
+      if (!empty($field_type['default_formatter'])) {
+        $configuration['type'] = $field_type['default_formatter'];
...
-    $configuration['settings'] += field_info_formatter_settings($configuration['type']);
-
+    if (isset($configuration['type'])) {
+      $configuration['settings'] += field_info_formatter_settings($configuration['type']);
+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php
@@ -110,7 +110,7 @@ public function form(array $form, array &$form_state) {
-          'defaultFormatter' => $field_types[$field['type']]['default_formatter'],
+          'defaultFormatter' => isset($field_types[$field['type']]['default_formatter']) ? $field_types[$field['type']]['default_formatter'] : 'hidden',

So now we could have fields without formatters? Looks like as side-effect API change

+++ b/core/modules/path/lib/Drupal/path/Plugin/field/widget/PathWidget.php
@@ -0,0 +1,143 @@
+    // Integrate with advanced settings, if available.
+    if (isset($form['advanced'])) {
+      $element['#type'] = 'details';

Magic attempt to integrate for node-edit page? I need similar for #731724: Convert comment settings into a field to make them work with CMI and non-node entities so widget needs different wrappers for entity form and field UI module provided "default value" widget

re: #114:

1) The $path variable in Path::delete() refers to a {url_alias} database record, which is loaded with the passed-in $conditions. If no record is found, then the Path::delete() method/operation passes on a Boolean FALSE (query result) as $path, which obviously breaks hook_path_delete() implementations, because they expect a $path array.

2) Yes. See #1849724: Avoid critical Field API limitations

3) Yes, I'm not happy with that magic futzing for the node edit form, but it was requested above. I hope it will be replaced with proper entity form layouts in D8.

Status:Needs review» Needs work
StatusFileSize
new22.26 KB

I just repeated the testing steps in #110 (standard install profile, URLs on node types). The field is working on nodes pretty much as I expect now:
- Appears on the pre-defined content types.
- The field works. If I edit the node and change the alias, the old one is removed and replaced with the new one. If I delete the alias on the node edit form, it is removed from the path table. If I edit the alias on the Path admin page, the updated alias appears when I edit the node. No errors or warnings.

I also tested the field that was automatically added to the Tags vocabulary. It works too, but it isn't in a details section like on the Node edit form. I guess that is OK.

I also added the field to the User entity and to a new node content type I created. I got some PHP warnings (see below).

I also looked at the Help page for the Path module (see below).

I also wiped my site and started over from the Minimal install profile, turning on the Path and Field UI modules. I got some more PHP warnings (see below).

Here is a list of things I think still need to be fixed, just looking at the UI from this patch (no code review):

a) hook_help() needs to be updated with some text about how to use this field [this is part of the Documentation Gate http://drupal.org/core-gates]. The current help says (in the Uses section):
Creating aliases
Users with sufficient permissions can create aliases under the URL path settings section when they create or edit content. Some examples of aliases are: ...

My suggestion would be to change this to:

URL alias field
A URL alias field can be added to content types and other entity bundles that support fields (such as taxonomy vocabularies and user accounts). This field allows users with sufficient permissions to create a URL alias when they are creating or editing content.

b) The fieldset (or whatever they're called) on the Node edit form for the field should not be called "URL path settings". It should be called either "URL settings" or "URL alias settings".

c) I went to Manage Fields on the "People" configuration (to add fields to the User entity), and added the existing Path field there (without changing any of the settings). When I viewed my user account, I got 4 identical PHP warnings:

Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 79 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).

I got the same warnings when viewing my user account whether or not there was any alias defined.

d) I created a new content type and added the existing field to it. When I created a new node of that type and saved it, I got the same warnings as in (c) when viewing the node.

e) In the Minimal install profile testing, I created a content type, added a URL alias field, and clicked on "Manage Display". I got the following PHP warnings:

    Notice: Undefined index: type in Drupal\field_ui\DisplayOverview->form() (line 159 of core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php).
    Notice: Undefined index: type in Drupal\field_ui\DisplayOverview->form() (line 176 of core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php).
    Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 79 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).
    Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 84 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).

f) In addition, the URL Alias field was in the Enabled section on Manage Display by default, although its format was set to "Hidden". That seems wrong. It should be in "Disabled" if its format is "". There was a Language field that was in the Disabled section by default, so it must be possible to do that.

g) I added a Description in the widget settings. It appeared twice on the content editing page: once above and once below the field. See screen shot "doubledescription"

h) When using the Minimal install profile and creating my own field for URL aliases, after adding a new node and ending up on the node view page, I had 4 PHP warnings:

    Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 79 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).
    Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 84 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).
    Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 79 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).
    Notice: Undefined index: type in Drupal\field\Plugin\Type\Formatter\FormatterPluginManager->getInstance() (line 84 of core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php).

Issue tags:+Needs tests

Incidentally, there should be automated tests doing all the things I've been testing manually, shouldn't there? They would have caught all the PHP notices I am seeing... Tests are also part of http://drupal.org/core-gates right?

Status:Needs work» Needs review
StatusFileSize
new10.48 KB
new55.06 KB
PASSED: [[SimpleTest]]: [MySQL] 50,655 pass(es).
[ View ]

Thanks for testing, @jhodgdon! Attached patch fixes all issues you raised in #116 and also adds test coverage for the steps you performed manually.

  1. Updated for string capitalization changes in translation_entity module.
  2. Fixed FormatterPluginManager still tries to format fields that do not have any formatters.
  3. Changed vtab label into "URL settings".
  4. Added web test for adding/editing a custom URL alias field (to user accounts).
  5. Fixed field help/description is duplicated into vertical tabs/details container.
  6. Fixed PHP notices in DisplayOverview; Path field does not appear as hidden + tests.
  7. Adjusted Path module help text.

+1 to RTBC, All pointed could be addressed in follow-up/clean-up issue

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -76,16 +76,27 @@ public function getInstance(array $options) {
+      // Switch back to default formatter if either:
+      // - $type_info doesn't exist (the formatter type is unknown),
+      // - the field type is not allowed for the formatter.
+      $definition = $this->getDefinition($configuration['type']);
+      if (!isset($definition['class']) || !in_array($field['type'], $definition['field_types'])) {
+        // Grab the default formatter for the field type.
+        $field_type_definition = field_info_field_types($field['type']);
+        if (!empty($field_type_definition['default_formatter'])) {
+          $plugin_id = $field_type_definition['default_formatter'];
+        }
+      }
+    }

This part requires approval of "field-ds" team (@yched & Co)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.phpundefined
@@ -107,7 +107,7 @@ public function build(array $form, array &$form_state) {
-          'defaultFormatter' => $field_types[$field['type']]['default_formatter'],
+          'defaultFormatter' => isset($field_types[$field['type']]['default_formatter']) ? $field_types[$field['type']]['default_formatter'] : 'hidden',
@@ -153,7 +153,7 @@ public function build(array $form, array &$form_state) {
-          '#default_value' => $display_options ? $display_options['type'] : 'hidden',
+          '#default_value' => isset($display_options['type']) ? $display_options['type'] : 'hidden',
@@ -170,7 +170,7 @@ public function build(array $form, array &$form_state) {
-      if ($display_options && $display_options['type'] != 'hidden') {
+      if (isset($display_options['type']) && $display_options['type'] != 'hidden') {

This two hunks too

+++ b/core/modules/path/lib/Drupal/path/Plugin/field/widget/PathWidget.phpundefined
@@ -0,0 +1,147 @@
+    // Integrate with advanced settings, if available.
+    if (isset($form['advanced'])) {
+      $element['#type'] = 'details';
+      $element['#group'] = 'advanced';
+      $element['#title'] = t('URL settings');
+    }

Only content(node) has this for now, probably needs a follow-up and @todo

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.phpundefined
@@ -30,19 +30,42 @@ public static function getInfo() {
+    /*
+    // Enable entity translation for the entity type/bundle.
+    translation_entity_set_config('node', 'page', 'enabled', TRUE);
+    drupal_static_reset();
+    entity_info_cache_clear();
+    menu_router_rebuild();
+    // Enable field translation for the body field.
+    $field = field_info_field('body');
+    $field['translatable'] = TRUE;
+    field_update_field($field);
+    // Enable field translation for the default path field.
+    $field = field_info_field('path');
+    $field['translatable'] = TRUE;
+    field_update_field($field);
+    */

This needs @todo and comment

+++ b/core/modules/path/path.installundefined
@@ -0,0 +1,39 @@
+function path_field_schema($field) {
+  $columns = array(
...
+  return array(
+    'columns' => $columns,

@yched suggested different style #731724-333: Convert comment settings into a field to make them work with CMI and non-node entities

+++ b/core/modules/path/path.moduleundefined
@@ -39,6 +39,154 @@ function path_help($path, $arg) {
+ * Creates a default 'path' field that can be attached to any entity bundle.
...
+ * @see path_add_default_field_instance()
+ * @see path_install()
+ */
+function path_add_default_field() {
+  $field = field_info_field('path');
+  if (empty($field)) {
...
+    $field = field_create_field($field);

Is not this better to make private function?

+++ b/core/modules/path/path.moduleundefined
@@ -39,6 +39,154 @@ function path_help($path, $arg) {
+function path_add_default_field_instance($entity_type, $bundle, $label = 'URL alias') {
...
+  $instance = field_info_instance($entity_type, $field['field_name'], $bundle);
+  if (empty($instance)) {
...
+    $instance = field_create_instance($instance);

Same here

StatusFileSize
new55.68 KB
PASSED: [[SimpleTest]]: [MySQL] 50,815 pass(es).
[ View ]
new3.02 KB

Added form alter to hide settings for field cardinality and cleaned useless variable in hook_field_schema()

EDIT Also we need UI follow-up to find a sane way to make path field translatable. This regression could lead to data loss when migrated data has language-specific aliases but field does not have translation enabled

Status:Needs review» Needs work

The last submitted patch, 1751210-path-120.patch, failed testing.

+1 RTBC from me

Some minor nitpicks that I'm happy to push to followup.

We just need ok from field_api folks (@yched or @swentel) on the formatter changes identified by @andypost in #119

+++ b/core/modules/path/lib/Drupal/path/Plugin/field/widget/PathWidget.phpundefined
@@ -0,0 +1,147 @@
+ * Contains Drupal\path\Plugin\field\widget\PathWidget.

shouldn't this be \Drupal

+++ b/core/modules/path/lib/Drupal/path/Plugin/field/widget/PathWidget.phpundefined
@@ -0,0 +1,147 @@
+   * Implements Drupal\field\Plugin\Type\Widget\WidgetInterface::formElement().

Same

+++ b/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.phpundefined
@@ -140,23 +133,23 @@ function testNodeAlias() {
+    $this->drupalGet($edit['path[und][0][value]']);

shouldn't this use LANGUAGE_NOT_SPECIFIED?

+++ b/core/modules/path/lib/Drupal/path/Tests/PathFieldCRUDUnitTest.phpundefined
@@ -0,0 +1,144 @@
+ * Contains Drupal\path\Tests\PathFieldCRUDUnitTest.

same (missing \)

+++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.phpundefined
@@ -55,42 +78,40 @@ function testAliasTranslation() {
+    $edit['path[und][0][value]'] = $english_alias;

same?

+++ b/core/modules/path/path.moduleundefined
@@ -95,224 +243,71 @@ function path_menu() {
-function path_form_node_form_alter(&$form, $form_state) {

oh yeah!

Installed and took this for a test drive. Works great.

Great work on this!

Status:Needs work» Reviewed & tested by the community

Spoke to @swentel about changes to formatter logic on irc

7:54pm larowlan:
swentel: basically - do you have any objection to additional logic that assumes 'no default formatter' for a field type is equivalent to 'hidden' in display settings?
7:56pm swentel
larowlan: not at this moment, certainly not to block this patch - we can always make that logic prettier in follow ups
7:58pm swentel
larowlan: it's something that's on our plate anyway, we need to also add the possibility to have formatters without widgets etc, but that's all post 18feb material

On that basis I think this is RTBC too

StatusFileSize
new55.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1751210-path-123.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new6.12 KB

Addressed doc-blocks and $lang_code
Also remove commented/unused code

Status:Reviewed & tested by the community» Needs review
Issue tags:+RTBC Feb 18
StatusFileSize
new36.01 KB

Hm. So I'm coming a little late to this party, but I don't quite understand why doing this is a good idea.

- Now "URL alias" is forever polluting the kinds of fields I can add to an entity. Text, Boolean, Date, Link, Number, ... URL Alias? "One of these things is not like the other..."
- There's nothing stopping me from adding two URL alias fields to an entity, but that never, ever makes sense.
- When you do this, btw, it seems the field weighted lowest in the managed fields form wins, although interestingly it shows the fields in the opposite order on the node edit form. Upon saving, both fields take the same value.
- Speaking of the node edit form, it auto-expands URL alias fields in vertical tabs. Why? I would expect it would only do this if I marked them as required.
Two auto-expanded URL alias fields
- When I added it to users and made it required, "Show on registration form" was checked, but the field never showed on the registration form.
- When I register the first user, it works fine, because it auto-generates using my default value of "user/". However, any subsequent user gets "The alias is already in use." and no way to change it.

It seems to me like if we want to generalize this behaviour (which overall seems like a sound thing to do), it needs to be an entity property, not a proper field. What am I missing? And why did we blow our chance to get pathauto in core over this? :(

Given what is going on with those other issues, should we just close this one as a duplicate?

No they are different but related #1980822: Support any entity with path.module just a clean-up, others just a way to friend fields with typed-data

Um. What does "friend fields" mean? And if the path module is using a completely different way for being able to do paths on all entities, isn't this issue here (about making it a full Field) irrelevant?

Category:feature» task
Issue tags:+D8MI, +language-content

@webchick: this is actually a *very* important piece of change for D8MI. If you translation enable a node now, the URL path settings will apply to "all languages" regardless of which language you edit. By making it a proper field and using the form language code as in the patch, it would support per-language alias editing within the node form, which is otherwise not possible or would require lots of custom code. We can write lots of custom code to make this property aware that it actually have multilingual storage in the path system but fields have that natively, so why duplicate this?

I don't consider this a feature, it unifies how we expose this data and uses the same API for this piece of data as well. This has the same level of importance for D8MI as #1911080: Replace menu node form additions with entity reference field regarding the same for menus. I'd strongly support moving ahead with this and fix the side issues you identified instead of scrapping it.

"Why duplicate this" is in #125. It makes absolutely no sense for URL aliases to be exposed in the user interface as fields that can be added to entities.

I was hoping this wouldn't require a lot of custom code, though. My understanding was the Entity Property|Field API was supposed to unify those two concepts so we could treat non-field properties the same as proper fields. It might be that this should be postponed on #1949932: [META] Unify entity fields and field API.

I'm fine with re-classification as a task, though.

I honestly care more about the usability of the actual front end UI (the node form) vs the backend UI. So this patch in my understanding fixes a major translation-applicability issue with the actual front end form. We can only postpone such issues for so long. This issue also goes with API changes and the unification of the entity fields and properties is not really progressing AFAIS. These issues will return and bite us as major or critical usability bugs sometime soon after API freeze if not fixed before then.

the unification of the entity fields and properties is not really progressing AFAIS

FWIW, I'm currently working on that in #1969728: Implement Field API "field types" as TypedData Plugins - I just can't dedicate as much time as I'd like to. Will most likely need some live debates with the EntityNG folks (@fago, @Berdir...) at some point, I think this should be ready-ish at the end of DC Portland (at least that's the main item on my agenda there).
Then there's also #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, that is seeing some progress.

Status:Needs review» Needs work

The last submitted patch, 1751210-path-123.patch, failed testing.

Assigned:sun» Unassigned

Will try to fight the issue at weekend. The only ugly this is need in form_alter for field settings to prevent modify cardinality.

@yched is there way to skip field settings screen or other way to prevent change cardinality for field?

We have link module and it's widget in core, so this widget could be re-used after #1969728: Implement Field API "field types" as TypedData Plugins so the only a part of previous patch will be needed

It's actually a bit more complicated than just the cardinality of the field (see comment #125 above):
- There shouldn't really be more than one field of this type in existence in the system.
- If there are multiple fields of this type in existence, there shouldn't be more than one on any given entity bundle.
- And the cardinality of the field should probably be one... although, is it not possible to have multiple aliases for the same base path?

All of which kind of means it needs to behave more like a property than a field... except that:
- It needs to be translatable (are properties translatable now?)
- You need to be able to decide on an entity/bundle basis whether it should be there or not. (only some entities and only some of their bundles need it, and some don't, so you don't want to just say it has to be there on all entities/bundles or have the entity decide or have the path module try to decide).
- It would be useful to be able to change the field/property label ("URL alias" is not actually something that most people who are not Drupal developers necessarily understand -- I'm always having to explain to less developer-savvy folks that it means a "pretty URL") (of course, if we did a quick usability survey on that maybe we could have the core label better in the first place).

So it kind of seems like the setting for "do you want URL aliases" should be a checkbox on the bundle's "Edit" screen rather than a field/field type that you can create/add.

Is that possible? Is that desirable? Can it be that way and still be a field (albeit a special "you can't create or add this" field that wouldn't show up in the "add new field to bundle" field type select list, or in the "add existing field to bundle" select list)?

@jhodgdon Thanx a lot for summary! Now it's much clear

1) need a field with 'no_ui' flag set
2) PathItem for FieldAPI
3) Settings screen - suppose admin/config/search/path/settings Tab a-la admin/config/regional/content-language to enable on per-entity-basis
4) Widget & part of current patch

RE #14-

1) I do not think that setting a flag to "no_ui" takes care of all of that? At least in D7, all it really did was make it so if you tried to edit the field settings, you got a not very helpful message saying you couldn't edit the field settings (even though the edit link was visible, and yes there's an issue). But maybe it is different in D8?

2) No idea what that means.

3) Not per-entity. It needs to be per-bundle, which is why I suggested putting it on the bundle editing screen (the one where you set up the human-readable name, description, etc. for the bundle, and in the case of nodes also configure things like whether it has revisions turned on by default, is published by default, etc.).

4) Presumably yes.

Another thought I just had about the "no UI" idea: We actually probably do want the ability for users to change the widget for this field. The major use case would be Pathauto -- the idea was that if Path became a field, Pathauto would just be an alternate widget for the field. Would setting this field to "no UI" still let people choose a widget on a bundle-by-bundle basis?

To add to #142, could a contrib (such as Pathauto) alter the "no UI" setting, in order to allow itself to expose the choice of an alternate widget than the default on a bundle-by-bundle basis?

Issue tags:+Needs reroll

Needs reroll

Issue summary:View changes

Updated issue summary.

RTBC Feb 18 2013. 9 more days to go.

"There's nothing stopping me from adding two URL alias fields to an entity, but that never, ever makes sense."

I have to disagree with that, I have use case where I need to have two aliases for each node. A video node designed to be referenced in Scientific Journals by document id. So it needs two aliases "Friendly URL" http://site.edu/video/%title% and reference url http://site.edu/%reference-id%

Admittedly this is an edge case, and I am doing it fine in Drupal 7 via Rules. It might lead to more confusion in the 99% case, so not be worth supporting, but there would be valid times to use it.

Note: This one reminds me of the @fago's comment on #731724-380: Convert comment settings into a field to make them work with CMI and non-node entities.

It would be great if we could manage the fields so that there were "regular" fields and "meta-data" fields ... if we had "meta-data" fields I think this issue would be a lot easier to land. (Could handling meta-data fields go into a Drupal 8 minor release?)

Status:Needs work» Closed (duplicate)

@Dustin@PI: While I wholeheartedly agree that there are certainly use-cases for associating multiple URL aliases to a single content entity, such a concept is not supported by the underlying path alias storage, which consists of a simple system path → alias mapping, and the language code is the only supported context that allows for multiple URL alias variations.

In addition, "multiple aliases per entity" only asks for a multiple value field, but not for being able to attach multiple URL alias fields (with a single value each) to an entity type. The latter is the part that makes little sense in the proposal to turn the path field into a Field-UI-configurable field.


Given the feedback here, and in light of how D8 progressed in the meantime, this issue is now a duplicate of #1980822: Support any entity with path.module (which I just re-rolled and which should be ready)

We might have to extract the field widget additions of this patch into a new issue, so that the default Path field widget can be swapped out with a different implementation (→ natively enabling #1844438: Allow part of the generated path to be set as a fixed prefix in core or contrib, without having to swap out the entire Path module)