Problem/Motivation
Since path aliases have been converted to entities and their forms to entity forms, we should deprecate the existing path.alias_storage
service because some of its methods are no longer used anywhere (save()
, load()
, delete()
, aliasExists()
, languageAliasExists()
, getAliasesForAdminListing()
).
Proposed resolution
Deprecate \Drupal\Core\Path\AliasStorage
and its service (path.alias_storage
), and introduce a new path.alias_repository
service containing only the methods that are used in the critical path of each request :
- preloadPathAlias()
- lookupBySystemPath()
- lookupByAlias()
- pathHasMatchingAlias()
The main advantage of having a separate service is that instantiating the entity storage is heavier, because it needs two queries on the last installed definitions key value store (besides requiring to load more and larger class files). On top of that, every looked up path alias would imply an entity load (even a load from cache has its own overhead) with all the related field item (list) objects needing initialization. Hence the need for a more lightweight system.
Full deprecation list for AliasStorageInterface
:
Existing method | Replacement |
---|---|
save() |
\Drupal::entityTypeManager()->getStorage('path_alias')->save() |
load() |
\Drupal::entityTypeManager()->getStorage('path_alias')->load() |
delete() |
\Drupal::entityTypeManager()->getStorage('path_alias')->delete() |
preloadPathAlias() |
AliasRepositoryInterface::preloadPathAlias() |
lookupPathAlias() |
AliasRepositoryInterface::lookupBySystemPath() |
lookupPathSource() |
AliasRepositoryInterface::lookupByAlias() |
pathHasMatchingAlias() |
AliasRepositoryInterface::pathHasMatchingAlias() |
aliasExists() |
No replacement. |
languageAliasExists() |
No replacement. |
getAliasesForAdminListing() |
No replacement. |
Remaining tasks
Review the patch.
User interface changes
Nope.
API changes
The CRUD related methods from \Drupal\Core\Path\AliasStorageInterface
have been deprecated in favor of the CRUD methods provided by the entity storage.
A few methods from \Drupal\Core\Path\AliasStorageInterface
have been moved to \Drupal\Core\Path\AliasRepositoryInterface
.
A few methods from \Drupal\Core\Path\AliasStorageInterface
have been deprecated without any replacement: aliasExists()
, languageAliasExists()
and getAliasesForAdminListing()
.
Comment | File | Size | Author |
---|---|---|---|
#106 | interdiff-105.txt | 6.78 KB | amateescu |
#106 | 2233595-105.patch | 111.74 KB | amateescu |
#96 | interdiff-96.txt | 20.88 KB | amateescu |
#96 | 2233595-96.patch | 110.6 KB | amateescu |
#91 | interdiff-91.txt | 8.89 KB | amateescu |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedComment #2
marcingy CreditAttribution: marcingy commentedAlright, we have a bit of a hack in path_load() for now but that is dying else where, depending on how that other issue progress I may well merge that into here.
Comment #3
slashrsm CreditAttribution: slashrsm commentedLooks great! Thanks! I basically have just one terminology related comment....
I'd use $path instead of $source. "Source path" is one of the names that are currently used for non-aliased path, but we agreed that we should simply use "Path" for that. Using correct variable names will help us to enforce this terminology.
Same as above.
As above.
As above.
Comment #4
visabhishek CreditAttribution: visabhishek commentedi am updating the patch as per #3
Comment #5
slashrsm CreditAttribution: slashrsm commentedThanks! Looks OK.
Note that this conflicts with #2233607: Kill path_load() and use \Drupal::service('path.alias_storage')->load($conditions) instead. We either have to merge the two patches or re roll when first of the two gets in.
Comment #7
visabhishek CreditAttribution: visabhishek commentedNow patch is updated as per #5.
Comment #8
slashrsm CreditAttribution: slashrsm commentedComment #9
slashrsm CreditAttribution: slashrsm commented7: issue-2233595-07.patch queued for re-testing.
Comment #11
marcingy CreditAttribution: marcingy commentedComment #12
marcingy CreditAttribution: marcingy commented7: issue-2233595-07.patch queued for re-testing.
Comment #14
marcingy CreditAttribution: marcingy commented7: issue-2233595-07.patch queued for re-testing.
Comment #16
marcingy CreditAttribution: marcingy commented7: issue-2233595-07.patch queued for re-testing.
Comment #18
marcingy CreditAttribution: marcingy commentedReroll and a couple of tweaks for migrations having an issue with the interdiff generation.
Comment #19
slashrsm CreditAttribution: slashrsm commentedComment #21
marcingy CreditAttribution: marcingy commented18: issue-2233595-18.patch queued for re-testing.
Comment #22
slashrsm CreditAttribution: slashrsm commentedComment #23
alexpottNeeds a reroll
Comment #24
visabhishek CreditAttribution: visabhishek commentedRerolled
Comment #26
marcingy CreditAttribution: marcingy commentedClean re-roll of https://drupal.org/node/2233595#comment-8740271
Comment #28
shumer CreditAttribution: shumer commented26: issue-2233595-26.patch queued for re-testing.
Comment #30
marcingy CreditAttribution: marcingy commentedOr not as the case was this should be green, I missed a method in the test that had changed.
Comment #31
slashrsm CreditAttribution: slashrsm commentedComment #32
catchThese all say 'fetches/deletes a specific alias' but what happens if there are two different aliases pointing to the same path?
Why are the keys removed here? Should remove the reference to them if they really don't need to be here.
The keys should be listed here.
Comment #33
benjifisherI updated the issue summary. I also tested locally (did not want to bother the busy testbot) and found that the patch does not apply. I think it should be easy to re-roll it: the directory structure has changed.
Comment #34
SpartyDan CreditAttribution: SpartyDan commentedI am re-rolling the patch.
Comment #35
SpartyDan CreditAttribution: SpartyDan commentedRe-rolled patch from comment #30. Does not address issues raised in comment #32.
I'm not sure that I understand the issues raised in #32. Please help me (novice) by elaborating on comment #32.
Comment #36
marcingy CreditAttribution: marcingy commentedSending too bot.
Comment #37
hanoiiI will be reviewing this patch now, please let me know if anybody else is reviewing it.
Comment #38
SpartyDan CreditAttribution: SpartyDan commentedI added documentation to patch #35 that addresses questions in comment #32. Patch and interdiff are attached.
The method will only return 0 or 1 records because it is querying the database for one unique identifier.
Comment #39
hanoiiKeys missing here in the documentation
And here as well
EDIT: Dreditor code properly updated
I will get to review the patch in more depth now, the concern on what happens with multiple aliases to the same path applies to the *ByPath() alternatives of the method, as there can be different aliases to the same path, I guess that should be somehow working and only needs documentation, but will try to get to review that as well.
Comment #40
SpartyDan CreditAttribution: SpartyDan commentedadd documentation as noted in #39
Patch and interdiff attached.
Comment #41
hanoiiWe reviewed the concerns of @catch about the possibility of having multiple aliases of the same path and although that's perfectly possible, the code doesn't currently account for that (and it never did on previous versions), the reason is on the code below:
Because the previous ->load() or the new loadByPath() and loadByAlias() rely on fetchAssoc(), it's OK for the code that uses these methods assume that only one record will be return and act accordingly. There is the possibility of having multiple alias of the same node, however, the possibility is never accounted on the code. I am unsure of what way to go from here.
The only place that loadByPath() is used by this refactor (and this still applies to the code without it) is on core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php, which is probably the widget that edits the path alias on the node.
In the case of multiple aliases for the same node (on the same language) one of the aliases will be likely pulled randomly from the db and used there.
I can think of different approaches here. The best one I think is to enforce some kind of uniqueness for a combination of language/paths, which would actually make a lot of sense. I would then remove the possibility to have a NULL language code be passed to the loadByPath() so that always a combination of path/language is returned, even if it's 'None'. This will prevent the function to be used to pull all aliases of a path for every language but that's certainly not being happening currently in the code, and if that's necessary, probably a new function can be created. If this is the way to go, new testcases will need to be added, which I can offer myself to do is this approach is preferred.
Comment #42
hanoiiComment #43
hanoiiUn-assigning from what I understand from DrupalCon Austin on how assignments should work.
Comment #44
hanoiiComment #45
a_thakur CreditAttribution: a_thakur commentedPatch in comment #40 does not apply to latest 8.x codebase.
Comment #46
a_thakur CreditAttribution: a_thakur commentedComment #49
carletexReroll done.
We have deleted the .orig file because I think it was a mistake, let me know if it was correct.
Comment #55
jsobiecki CreditAttribution: jsobiecki commentedI'm working on re-roll
Comment #56
jgeryk CreditAttribution: jgeryk commentedgoing to work on reroll
Comment #57
Sharique CreditAttribution: Sharique as a volunteer commentedRe-rolling patch.
Comment #59
Sharique CreditAttribution: Sharique as a volunteer commentedFixed syntax error from previous patch.
Comment #61
Crell CreditAttribution: Crell at Palantir.net commentedThis isn't an 8.0.x-safe issue as it's an API change.
What we could do in 8.1 is leave load() as is, but add the other methods including a loadById(). That would keep the current API intact. That has to wait for 8.1 though.
Comment #68
jibranThe reson mentioned in #61 to postpone is not valid anymore but we can postpone this on #2336597: Convert path aliases to full featured entities.
Comment #69
jibranComment #70
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIn light of #3007661: Modernize the path alias system, I'd like to re-purpose this issue to deprecate the existing path alias storage. Still blocked on #2336597: Convert path aliases to full featured entities.
Comment #71
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis should be everything needed to deprecate the existing path alias storage. This patch applies on top of #3011807: Convert the path alias admin overview to a list builder because there are a lot of usages of the deprecated storage that have already been converted in #3007832: Convert custom path alias forms to entity forms and #3011807: Convert the path alias admin overview to a list builder.
Comment #72
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAll the dependent patches were updated, so this one needed a reroll too.
Comment #73
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed most of the fails and brought back some tests because the deprecated alias storage still needs to be tested until we remove it.
Comment #74
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice, the only fails left are the migration ones, so I posted a patch over at #3009014-2: Convert path alias migrations to use entity:path_alias destination. Let's try to include that one in the combined patch.
Comment #75
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedPP-1 is wishful thinking, this is PP-6 at the moment but it's easier to not try and keep track of that number anymore :)
Comment #76
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAlso needed a reroll.
Comment #77
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedUpdated the patch to point to the change record.
Comment #78
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAlso rewrote the issue summary.
Comment #79
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed the remaining deprecated usages in migrate tests and updated the queries in
AliasStorageTest
to use an entity query instead.Comment #81
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRerolled on top of the latest patches from #2336597-136: Convert path aliases to full featured entities and all of its followups.
Comment #82
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRerolled again and fixed the last relevant test fail.
Comment #83
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRerolled for all the recent changes in the parent issues.
Comment #84
jibran#2336597: Convert path aliases to full featured entities is in.
Comment #85
Chris Matthews CreditAttribution: Chris Matthews commentedComment #86
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNew patch that applies on top of:
#3007669: Add publishing status to path aliases
#3009014: Convert path alias migrations to use entity:path_alias destination
#3007832: Convert custom path alias forms to entity forms
#3011807: Convert the path alias admin overview to a list builder
Comment #88
BerdirWhat are your thoughts on making this a new service vs. just deprecating the things we no longer need on AliasStorage?
I would have assumed that to be the easier change?
Either way, we should probably document here a bit why this is a separate service that is accessing the table directly and not using the entity API?
Not 100% sure about deprecation messages and how to handle that exactly here. If we AliasStorage, then we need to add @trigger_error() to each actually deprecated method including legacy tests. With a new service, you might be able to skip @trigger_error() on the methods, but we will still need legacy test for the code, *including* for the methods that are really just moved over here.
Hm. This should be possible as an entity query too with a STARTS_WITH condition?
It's used only by AliasWhitelist which in turn is basically a private service for AliasManager. (Literally the *only* call, there not even a test using this method)
And while it may be called during the critical-path alias-lookup, it's only called once for the root parts of known routes and as well cached as technically feasible ;)
DI is a bit tricky, as the alias services need the entity type manager then, but we only need to initialize the storage with the extra queries on the installed definition if we actually have a miss.
*If* we go with a separate service then maybe we should then align the arguments and method names with the new field names of the PathAlias entity now?
Comment #90
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed the tests for now.
Comment #91
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRe @Berdir in #88:
1. As discussed, the reason for introducing a new service is to improve the method names and return values for
lookupPathAlias()
andlookupPathSource()
.2. I would prefer to keep the existing query as-is. We can always deprecate this method later if we are comfortable with the slight performance penalty.
3. The new methods are already improved :)
Added full test coverage for all the methods of the deprecated
AliasStorage
service, and updated the IS as well.Comment #92
plachThe blocker is in!
Comment #94
BerdirFirst chunk of review.
Found two subclasses of this: http://grep.xnddx.ru/search?text=extends%20AliasStorage
By deprecating and no longer using this, we break those modules, but that's always the possibility when deprecating a service/method. It's easier with functions, you can't subclass/replace a function ;)
This needs BC, probably by removing the type and then checking what it is and if we don't like it, do a @trigger_error(). And probably use the DeprecatedServicePropertyTrait for the storage property.
Didn't find a subclass of this, but I did find a decorator/duplicate: http://grep.xnddx.ru/search?text=implements+AliasManager&filename=
This might be a problem.
I applied the patch locally, then tried to do a drush cr. Problem is that apparently drupal_flush_all_caches() does the twig clearing stuff before rebuilding the container, and the twig service has a nice dependency chain through the url_generator, through route provider then all the path processors down into this.
Suggestion: If you happen to be in that really weird state, having working aliases is probably not a priority, so we could not fall back here and return nothing in these methodss if we don't have a service? Or we keep the old implementation as fallback. Is there a reason you replace it (the workspace replacement?) or just to avoid duplicated code?
Same here, BC..
Comment #95
BerdirReally like the trait, makes these tests much nicer.
Wondering if prefixing the class with Workspace would make sense, thought about that before too. Would avoe the use X as Y easier to find the actual implementation in an IDE.
This could be the place to switch to setter injection to not having to touch the constructor, per my comment in the published status issue. But still not very important ;)
I think it's more common to name these classes LegacyXy, that's already filtered out then be our tools look for deprecated method calls.
Also, we should have some @expectedDeprecation things here.
I'm not quite sure how that will work as we just have the one on the service and you already get that in the constructor. Maybe that will still work if we just pot that on every test method? Even less sure about the one in the class file as that will only be loaded once I suppose.
we can remove $this->aliasStorage?
null isn't the actual default though, that would be undefined as the implicit default or not? so maybe document as that? Or just make that the explicit default and remove the conditional?
Comment #96
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks for the review!! Fixed all of #94 and #95.
For #94.3: I wanted to avoid duplicated code, but since the deprecated service has full test coverage in
LegacyAliasStorageTest
, I guess it's better to keep the old code in place so we can keep the upgrade process as smooth as possible.Comment #97
BerdirIt's a big patch but I I think this is ready. Reviewed it in depth before and the interdiff now looks good.
As commented in #94, fully deprecating the service will "break" a handful modules that subclassed/replaced this service. But that's partially already the case as we already no longer call these methods in the places that matter (This mostly updates tests). And we also missed the alpha deadline and this does technically add more deprecations. However, it is the last patch in a series of steps that got in, and not committing this would leave us in a pretty weird state for Drupal 9, with two different, not-officially deprecated API's to do the same things.
There would theoretically be a version of this that would be smaller, by only deprecating the removed methods and not removing the alias storage service completely. The new service allows us to improve DX with better named methods and also improved performance, specifically in PathFieldItemList, we can save an additional query because all information we need is already returned. It would also require additional work/time to go back to that.
So, I guess it is up to the RM/FM's now (that sounds like a radio station, with @xjm as the DJ)
Comment #98
jibran#3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations might be blocking this.
Comment #99
catchI am pro backporting this during alpha (assuming it's ready, I've not reviewed the whole patch yet), since it is part of a set of changes that otherwise would have required a 500kb patch, rather than a new deprecation that missed the deadline. The disruption is already pretty much there in practice, and it will help to reduce confusion in 9.x. However making sure xjm gets a chance to weigh in here so tagging.
If we don't backport it to 8.8.x and only commit it to 8.9.x, jibran is right it will end up blocked on #3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations until we have a plan there.
Comment #100
xjmWelcome back to RM FM radio, streaming to you live on this brisk fall morning. Expect unfrozen commits, with a chance of upgrade path regression in the path alias storage. Now, some mellow tunes to help you unwind after a long week of late hours.
Comment #101
xjm(...I think this in theory is better to do sooner rather than later given the risk of having both around, but would like to give it a read first since it's a big patch.)
Comment #102
xjmYeah this seems like something we should do between alpha and beta for the reasons described in #99. Let's make sure to document it clearly for the beta release notes, though.
Why is this one changing from 2 to 8? And why didn't the test fail for that?
Comment #103
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@xjm, the test is not failing in HEAD because
\Drupal\Tests\path\Kernel\Migrate\d6\MigrateUrlAliasTest::assertPath()
does not assert the path alias ID. However, this patch adds an assertion for the ID in that method, and the correct value (8) can be seen incore/modules/migrate_drupal/tests/fixtures/drupal6.php
- search forsource-noslash
:)Comment #104
plachAside from the nits below, my main concern about this is that
AliasRepositoryInterface
andAliasManagerInterface
have very similar methods with very similar descriptions and it’s not easy to figure out at first sight which ones to use. Looking at the code AMI implementations depend on ARI ones, so it’s clear AMI should be used. However, at very least we should mark the repository as@internal
, but probably a follow-up providing additional docs would help.This is mentioning entities: we should either change it or ditch the ArrayPI ;)
Not introduced by this patch but we should probably update these comments.
Same here.
Removing the RM tag, since both @xjm and @catch agreed on this being ok to do ASAP.
Edit: typos
Comment #105
plachDocumented a few details about the proposed solution in the IS.
Comment #106
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks for reviewing! Fixed all the points from #104 and added (hopefully) very clear documentation for the new service.
Comment #107
plachPatch looks good to me, thanks!
Comment #108
catch@plach mentioned this in slack:
Fixable on commit - putting it here so it's not forgotten.
Comment #112
catchCommitted 9d58bed and pushed to 9.0.x, then cherry-picked to 8.9.x and 8.8.x. Thanks!
Had forgotten this issue was five years old until going through issue credit, nice to close it out finally!