Problem/Motivation
Install standard.
Add a page node, with an alias via the node form.
Add a second page node, with no alias.
Then add an alias for admin/config/search
Expected result:
Both aliases have either 'en' or 'und' language codes. Both aliases can be edited.
Actual result:
Aliases added via the node form have 'en' as language code - can be edited.
Aliases added via the path alias UI have 'und' as language code - cannot be edited from the node form because the langcode doesn't match.
Proposed resolution
When looking for aliases for the node form, fall back to language neutral if there is not an alias in the language
Remaining tasks
User interface changes
API changes
Data model changes
Data migration may be needed.
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff-37-42.txt | 789 bytes | zaporylie |
#42 | 2511968-42.patch | 4.96 KB | zaporylie |
#37 | 2511968-37.patch | 4.93 KB | zaporylie |
#37 | 2511968-37-TEST-ONLY.patch | 1.91 KB | zaporylie |
#33 | 2511968-33.patch | 4.91 KB | jhedstrom |
Comments
Comment #1
dawehnerGiven that 'en' is used as the langcode of the entity itself, I think there is no reason that the path alias should not inherit that automatically.
Given that the same UI behaviour should happen for the path alias as well, always at least have one langcode available.
This could also ease the transaction into path aliases as entities in the future.
Comment #2
catchDuplicate of #2484411: Manual path aliases are not the same as aliases on the node form on single-language sites
Comment #3
catchRe-opening this after a discussion with @xjm @effulgentsia @alexpott @plach @swentel @berdir @amateescu
The path alias form should look for language neutral aliases since that's valid.
Comment #4
catchComment #6
BerdirComment #7
BerdirThat respects the langcode and falls back to unspecified.
Comment #8
BerdirForgot to add some assertions for the overview page.
Comment #9
dawehnerI'm wondering whether we should provide an update path and set the langcode on there
nitpick: we could add a "public" here.
Comment #10
Berdir1. set /where? This is a computed field type, this information not persisted anywhere anymore. It also wouldn't strictly be required, the patch works without it. But we will eventually also support loading an existing alias I hope, and then it might become useful.
2. Yeah :) I blame the existing methods which don't have this for forgetting it.
Comment #11
dawehnerOh right, nevermind.
Comment #12
alexpottThis change looks odd. Should
\Drupal\path\Plugin\Field\FieldType\PathItem()
override\Drupal\Core\Field\FieldItemBase::getLangcode()
and contain this logic? Maybe getLangcode() needs to always return the parent langcode - not sure.Comment #13
BerdirgetLangcode() is the langcode of the entity. It is a public method, overriding that feels wrong.
getLangcode() follows specific rules. You get the active translation, but if the field is not translatable then it is the language of the original translation. It is never not specified unless the entity actually has not specified as the langcode.
There will be additional changes with #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations, basically, if the field is not translatable then we will use language not specified and not the language of the original entity as the default in the widget, so that the alias is available in all languages.
It made sense to me to split that up, if you think it makes more sense to combine the two issues, then we can also do both things together. The other issue would basically do a isTranslatable() check and if not, us not specified. I'm not sure yet about changing it when saving or respecting the existing langcode, a this now does.
And I kept the getLangcode() as a BC fallback.
Yet another related issue is loading lazy loading existing aliases when accessing the data, which will mean support for REST, search api, rules and other systems which use field api/typed data in a generic way.
Back to you for feedback, feel free to set back. @catch might also have thoughts on this, as he opened the issue.
Comment #14
alexpottAbove are the docs on \Drupal\Core\Field\FieldItemInterface::getLangcode() - so whilst this is normally the same as the entity's langcode in this case it does not appear to be because PathItem has its own langcode. Tricky.
Comment #15
catchLet's add comments for #12/13 since it's a bit confusing, but that looks right to me.
Comment #16
BerdirTried to document this behavior a bit but failed to come up with a concise description why we didn't override getLangcode() (which might prove that it would maybe make sense? I'm not sure..)
While thinking about this, this issue, in combination with #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations could result in some interesting problems. Imagine a single language site, the path field is not translatable. With that other issue, we save the alias initially as unspecified. Then we add another language later and enable translations, making the field now translatable. Now we will find the existing unspecified alias when adding a translation and will update that instead of saving a new one in the translation language. We can figure out a solution in the other issue, possibly make the langcode visible to the user, so he can change it.
Comment #17
dawehnerSometimes it would be nice to document when to use this new langcode and when not.
In case someone wants to, we could store
\Drupal::service('path.alias_storage')
as local variable.I'm wondering whether we should test also the aliases directly, like going to the path aliases itself.
Comment #19
jhedstromNeeds a reroll as some of the tests have been moved to phpunit.
Comment #20
BerdirIt's a bit more complicated than that :)
"Rebased", which is more like a rewrite as this now can and has to be in PathItem instead of the widget.
The thing now is that the duplicated load always happens on accessing the value, which is likely more often than the edit widget.
Thinking about extending load to support array values in the condition and then have the same language sorting logic as the lookup methods.
Not addressed the review yet.
Comment #22
BerdirComment #24
jhedstromThis loads the first alias found of the 'und' language, since
$conditions
isn't previously set.This patch resolves that, but we need further test coverage since this was previously green.
Comment #25
jhedstromHere's an updated test that demonstrates the issue with #22. The 'test only' patch is actually that patch with only the updated test, and the full patch contains the updated test and the change in #24.
Comment #27
ptmkenny CreditAttribution: ptmkenny commentedJust a note: at the moment, this patch conflicts with the latest patch in 2689459. It would be great to either get this committed so 2689459 can be re-rolled or to combine the patches so that they can be used together.
Comment #28
jhedstromThe patch is also not compatible with the recent commit in #2916300: Use ComputedFieldItemListTrait for the path field type so needs a refactor.
Comment #29
jhedstromI'm going to work on the refactoring of the fix.
Comment #30
jhedstromHere's the refactored fix (no interdiff since the previous patch no longer applied).
Comment #33
jhedstromThis fixes coding standards. The fail in #30 looks to be unrelated/random:
Exception: Warning: apcu_store(): Unable to allocate memory for pool.
Comment #35
WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commentedThis has two failures on the 8.6.x branch.
Comment #37
zaporylieReroll of the #33.
Comment #38
Wim LeersI believe this would also address the unexpected/confusing behavior reported against JSON:API in #3012019: Include the path / url alias and #3015566: Missing path attribute?
This approach is new as of #20, and means the issue title should probably be updated.
Comment #41
zaporylieIt seems like we set the the language property of the PathItem to Drupal's default in PathFieldItemList::computeValue() when we create the form, so the $this->language is always populated and it doesn't respect in which language the entity is created, hence the Test error.
Comment #42
zaporylieThis patch fixes #41 by checking if path alias has pid value before using the langcode property.
One problem still remains - when url alias field is configured as non-translatable, whenever you add an entity translation it will create a new url alias in the source language of the entity. Ex. if I create node in english, set the path alias, then decide to add a polish translation and save a form I get two rows in the DB:
I believe it should be fixed in #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations as it only applies to the non-translatable aliases.
Comment #44
zaporylieNot sure why CI decided to fail on this - all tests are green now.
Comment #45
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commentedThis looks good to me, would be great to get the ball rolling and improve url alias handling.
Comment #47
BerdirThere is still the performance aspect to consider here. I'm wondering if this could be done like the main alias lookup. Look for both language specific and neutral alias in a single load, if there is more than one alias, prefer the one with the matching language.
Comment #48
Wim LeersThis would indeed be much better for performance.
Comment #49
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat would be very easy to do after #2336597: Convert path aliases to full featured entities since we'll be able to use an entity query with an OR condition on the langcode field :)
Comment #50
zaporylieDo you mean an additional method in AliasStorageInterface that combines
load
andlookupPathAlias
together? That sounds like a BC break so I guess we would need another service for that?#49 would be great but probably won't go into core before the next minor release?
Comment #51
zaporylieI'm also not sure about the performance impact - the code doesn't seem to run in the critical path (does it?). All API additions we may wanna add are soon to be deprecated by #2336597: Convert path aliases to full featured entities. All that makes me wonder - wouldn't be enough to just leave a comment and create the followup to refactor this part once #2336597 gets in?
Comment #52
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAgreed with #51, I think the current patch is fine for 8.6.x given that we know that it can be improved in 8.7.x.
Comment #53
catchIs there an issue open for the 8.7.x follow-up? I think it's OK for this to go in as a bugfix to both versions then optimise to a single query there.
Comment #54
zaporylieDone.
Comment #56
catchCommitted 7758b75 to 8.7.x and cherry-picked to 8.6.x. Thanks!
Comment #58
BerdirThat was fast ;)
> I'm also not sure about the performance impact - the code doesn't seem to run in the critical path (does it?
I'm not 100% sure about that. I did just check that at least by default, it doesn't do that, but because this is an implicit load of the field, all it takes is some code somehow accessing it to trigger that.
But fine :)
I did comment in the follow-up that I disagree with @amateescu about the entity query, though :)