I thought this issue already existed, but couldn't find it. If someone else knows where it is, please mark one or the other a duplicate and merge.
The path alias lookup system needs to turn into a self-encapsulated service object.
- It should be a single object that takes a database connection in its constructor.
- It should offer two public lookup methods: getSystemPath() and getAliasedPath() (or something). These map 1:1 to drupal_get_normal_path() and drupal_get_path_alias(), respectively.
- There should be basic crud methods on the object, too. These should mostly be a direct port from the current code in path.inc.
- There *must* be an interface defined for all of the above.
- The path object must be added to the DIC.
- The PathSubscriber object should be instantiated via the DIC, and have the path object injected into it via the DIC.
- PathSubscriber should be updated to use the injected object rather than the current hard-coded functions.
There should be unit (not system, unit) tests for all of the above classes.
Path alias lookups are highly optimized having been a cause of various performance issues in releases prior to D7, any patch should be properly profiled/benchmarked before commit.
Who wants it? :-)
while discussing at catch's patch at #1209226: Avoid slow query for path alias whitelists in IRC, i raised the idea of making the path lookup code into a pluggable class. rough interface idea being:
interface DrupalPathRegistryInterface {
function save(array $path);
function load($pid);
function delete($pid);
function getNormalPath($path, $path_language = NULL);
function getPathAlias($path, $path_language = NULL);
}
Comment | File | Size | Author |
---|---|---|---|
#98 | 1269742_98.patch | 1.56 KB | chx |
#86 | 1269742.alias_manager.86.patch | 99.18 KB | katbailey |
#86 | interdiff.txt | 3.5 KB | katbailey |
#83 | 1269742.alias_manager.83.patch | 99.38 KB | katbailey |
#83 | interdiff.txt | 17.33 KB | katbailey |
Comments
Comment #1
catchIf we do this it'll allow for the possibility of having pluggable storage for path aliases cleanly, which would be really nice to have.
I'd want to keep the whitelist handling as a separate class as per #1209226: Avoid slow query for path alias whitelists, since that's self-contained and potentially re-usable logic across different path alias implementations.
Possible implementations we could do with this:
4.5/6 "grab all aliases across the site" - for sites that only have dozens of aliases.
4.7-6.x "one query per path" (but probably using the whitelist)
What we have now in 7.x with a per-page cache of system paths + the whitelist.
Redis and/or MongoDB to take the paths out of SQL entirely.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedI would like to remove all this from the bootstrap code, and move that to a module. I see no reason at all to have a
path.inc
when we havehook_url_inbound_alter()
andhook_url_outbound_alter()
.Comment #3
catchThere's an issue for that at #464164: Move URL alias functionality into a Path API module (still separate from the Path UI) already.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedi've started on this, very early work is here:
http://drupal.org/sandbox/justinrandell/1272704
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i tried helping over at #1209226: Avoid slow query for path alias whitelists, and it just made me want to smash things.
so i smashed up path.inc.
this is really just some brute force refactoring, that adds a DrupalPathRegistryInterface, and a default implementation, DrupalPathRegistry, with a path() factory method. path CRUD functions use the new DrupalPathRegistry class, as do drupal_get_normal_path() and drupal_get_path_alias().
i like the idea of making path.inc a module, but i've wanted to get going quickly, and do this in stages.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedthis time without the drupal_clear_path_cache() fatals.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedwithout the syntax error in path.test.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedfix typo in DrupalPathRegistry->delete(), restore function signature of drupal_get_path_alias().
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedahem.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedok, more refactoring, prepare to get rid of drupal_lookup_path() by eliminating all calls to it that don't come from path()->getNormalPath() or path()->getPathAlias().
Comment #13
andypostHow this class factory will affect a simpletest? Do we need a way to reset this static?
Any reason to left this in path.inc? Suppose this sub-system could be pluggable so menu loader should not live in pluggable include
Comment #14
Crell CreditAttribution: Crell commentedbeejeebus asked me to note here that path resolution is a core function; In the Context API (not yet committed but expected to be eventually), the path:system context key is provided by core so core must provide at least one implementation for path resolution, even if it's essentially a no-op implementation.
However, it should be possible for contrib modules to change the handler used for a given key. (The exact mechanism for that is still being debated and may be dependent on the CMI.) In that case, core could either provide a default SQL-based implementation object or a no-op object, and a contrib module could specify some other implementation to replace it. It would not be using the WSCCI plugins system (as that will likely depend on context), but the swap-out mechanism is still available.
Having an interface for that resolution logic for classes to implement is a good idea either way.
Comment #15
sunGiven that we don't need and don't want and can't support this functionality in early bootstrap and in the installer/updater anyway, moving it into path.module actually makes most sense to me (as proposed in #464164: Move URL alias functionality into a Path API module (still separate from the Path UI)).
It could still be pluggable even if it's in Path module, so leaving this issue open.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commented.
Comment #17
catchMarked #1591632: Make path system OO and injectable as a duplicate.
I copied Larry's issue summary from there, but I have serious reservations about the feasibility of doing some of them in a first pass on this, you never know though so I'll hold off trying to explain them for now.
Comment #18
RobLoachIf we have the DrupalPathRegistry instance held in
drupal_container()
, we could have a parameter registered to allow the Path system to be swappable.It is easy to switch parameter definitions so that the correct path class is instantiated. Tagging so that this issue is tracked.
Comment #19
Crell CreditAttribution: Crell commentedWe don't need to split the class out like that. Different classes would likely need different setup anyway (different dependencies, say for one that stores the path index in MongoDB it would need a Mongo connection, not an SQL DB connection).
The first 4 bullet points in the summary can be done now with straight up unit testing (I've verified that you can do pure unit tests even with a DB connection as long as you write your code properly, it's quite easy) without even thinking about the DIC. We should do that. Then the rest of the bullet points are done after that class is written. At that point it's just wiring.
Build the component first, then wire it up.
Comment #20
katbailey CreditAttribution: katbailey commentedFor the sake of getting this issue moving again I have rerolled beejeebus' patch from #12 and moved the class and interface definitions into their own files under core/lib/Drupal/Core/Path. Seems like it still constitutes a decent starting point. I guess the main task for now is to add a constructor that takes a db connection object and then make it use that directly in its methods rather than using the various db wrapper functions. Is that about right?
Comment #22
Crell CreditAttribution: Crell commentedThanks, Kat! This gets us moving again. Yes, a connection parameter for the constructor is step 1. Review below:
I wonder if this method shouldn't be renamed. I have no idea what "normal" means. (Insert obvious joke here.) getSystemPath()? (Since system_path is what the property on the request object is called.)
I'm inclined to skip this factory entirely and just go straight to the DIC. Let's not make more work for ourselves by having yet another static singleton to track when we already have a better system for that.
Are we even using this function still? I think it's probably vestigial and should be removed. This logic has all been ported to listeners already. Or if not, it should be.
Just from the path, I don't actually understand what this function does. The docblock is getting lost somewhere. :-) Honestly I would rather avoid dumb-wrappers like this, as it only encourages us to keep using function singletons that break encapsulation and testability.
Class/Interface names should not have "Drupal" in them.
I know this is a straight-port, but I want to try and avoid anonymous arrays as data objects. If Path is not going to be its own data object, then the CRUD functions should take multiple useful parameters rather than an anonymous data array.
I have no idea how we're going to handle calling hooks from inside an injected object. :-)
One of the advantages of this approach is that we can replace these functional tests with direct unit tests of the path object. Those are blazingly fast. Have a look at the routing branch in the WSCCI sandbox for how I'm doing that there, including with a database connection. It works surprisingly well.
-1 days to next Drupal core point release.
Comment #23
katbailey CreditAttribution: katbailey commentedOK, getting closer, but I still haven't fixed the tests. Putting this up anyway in case someone else wants to work on it as I probably won't be able to for the next couple of days...
Comment #24
Crell CreditAttribution: Crell commentedComment #26
katbailey CreditAttribution: katbailey commentedFyi the latest work on this is now in a branch in the wscci sandbox: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path... (corresponds to the patch in #23).
From today's irc meeting:
[09:45am] katbailey: the path alias thing isn't really a use case for this because it's not a module
[09:45am] Crell: Perhaps we could move it to the path module, just to see if it can be done?
[09:46am] katbailey: sure, I can try that as a proof of concept
[09:46am] Crell: Ideally with this approach that functionality could live in a module without any downside.
[09:47am] Crell: Which brings me to... http://drupal.org/node/1269742
[09:47am] Druplicon: http://drupal.org/node/1269742 => Make path lookup code into an pluggable class => Drupal core, base system, normal, needs work, 25 comments, 7 IRC mentions
[09:48am] Crell: There's a very clear roadmap there for what needs to be done, and katbailey's already done a lot of it.
[09:48am] Crell: Who wants to finish it?
[09:48am] jmolivas joined the chat room.
[09:48am] katbailey: Crell: that confuses me - what about your comment here http://drupal.org/node/464164#comment-5922494
[09:48am] Druplicon: http://drupal.org/node/464164 => Move URL alias functionality into a Path API module (still separate from the Path UI) => Drupal core, path.module, normal, needs work, 29 comments, 14 IRC mentions
[09:49am] Crell: katbailey: That's before I realized that using an event listener for it, we could move it out to a module without any performance impact.
[09:49am] Crell: Well, without any serious impact.
[09:49am] Crell: Basically it's already been moved to a "hook" (event), so whether that lives in a hard-coded core list or a generated contrib list doesn't make much difference.
[09:52am] katbailey: ok, then if someone wants to take up the path aliasing stuff before I can get to it (which probably woudn't be for a few days) they could work on moving it out into a module, plus fixing up the tests of course
Comment #27
katbailey CreditAttribution: katbailey commentedI've recreated the path-alias-logic branch in the sandbox as it needed to be rebased against upstream/8.x. I also pushed a change to it that moves all the path alias lookup logic into path module. The core Path component now just has a stub implementation of PathRegistryInterface.
I'm not submitting a patch for this here yet because for this to work we need #1599108: Allow modules to register services and subscriber services (events). So I've added another branch to the wscci sandbox, path-bundle, that combines these two issues and shows how it would work: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path...
Here's a gist of the entire diff: https://gist.github.com/3068825
I'm not sure if it makes sense to postpone this issue until the bundles patch gets in. Maybe for now we should just keep it all in the Drupal\Core\Path component..?
Comment #28
Crell CreditAttribution: Crell commentedPer discussion with Dries at MWDS:
- #1599108: Allow modules to register services and subscriber services (events) breaks path alias caching, because of scope issues.
- The real fix for that is this issue, where we refactor the path alias library to be cleanly injected and not rely on globals that go out of scope on us at inconvenient times.
- Refactoring the path alias system as part of that issue would harm too many kittens.
- We're therefore OK with committing the bundle patch (above), but in exchange we mark this issue as a critical to restore path alias caching.
Comment #29
katbailey CreditAttribution: katbailey commentedThis still needs a lot of work but I want to get the patch up now anyway, seeing as I was able to reroll it against HEAD which now has bundles :-)
I'm noticing one particular set of tests exploding spectacularly (LanguageUILanguageNegotiationTest - I narrowed it down to when language_save() is called but am clueless beyond that) so testbot might not even give us an accurate idea of where we're at with tests, but I'd like to get feedback on the general approach before continuing with it...
Comment #31
katbailey CreditAttribution: katbailey commentedgah! Let's try this one...
Comment #33
BerdirProbably the completely wrong issue to discuss this and this might have already been done, haven't followed this part of core development closely.
This is really long, and you not only need to remember an arbitrary string identifier but also a method for which you don't have autocomplete.
What is this supposed to be look like finally? wouldn't it make sense to keep the old function as a wrapper for this, maybe temporarly?
Wouldn't clearCache() be a better method name?
Powered by Dreditor.
Comment #34
Crell CreditAttribution: Crell commentedIdeally the "final" version will have $path_alias_manager objects injected into other objects by the DIC, so you'd be doing $this->paths->getPathAlias(). Also, most code should never be touching that object directly.
No privates, and methods should be camelCase().
Statics inside a class are basically useless. They're shared between ALL instances of the object, which is rarely what you want. Instead you want $this->cache as an array or something.
For tests, I don't look at them closely but tests of the path alias object itself should not use drupal_container() but simply test the object directly, which is unit-testable then. (Or, should be.)
I don't think path_registry is the right name for this service. It's not a registry. It's an aliasing tool, and one of many possible ways to mess with paths. Essentially, it's one particular implementation of hook_url_inbound_alter() (at least the way it's wired in now via a kernel.request event). There could be many others, including pattern-based aliasing rather than a big denormalized lookup table.
For the same reason, while I understand why there's a null implementation in core with an override in path.module that actually does something useful, I'm not sure that's the right approach. Of course, the right approach may not be possible until we have #1705488: Implement a generator for Drupal paths, and some sort of event/hook/thing there that we can latch on to in a performant way. (I don't know what that performant way is yet.) So, that makes me uncomfortable but I don't know what the better alternative is yet.
Also, dear god when did the aliasing system get so bloody complicated?
Comment #35
catchI'm not at all sure it makes sense to combine the alias/system path lookup methods in the same class as the path crud functions. The alias/system path lookup is going to be necessary for any implementation we have, but saving individual path aliases isn't necessarily and even if you did we might not use pid etc. Also they're performing very different jobs already, in the same way efq and entity_*() are different.
Note there's a patch at #1209226: Avoid slow query for path alias whitelists which moves the path whitelist to DrupalCacheArray. Looking at the patch it's not changing any of this logic so it doesn't really matter but just cross-linking.
Comment #36
pwolanin CreditAttribution: pwolanin commentedTaking a pass at this while Kat is on the plane - sounds like we need to split this into 2 interfaces/classes for lookup vs crud?
Re-rolling for a start, since the patch doesn't apply to HEAD.
Comment #37
pwolanin CreditAttribution: pwolanin commentedsetting to needs review just for testbot
Comment #38
pwolanin CreditAttribution: pwolanin commentedmissed the new files
I just added this patch to a new WSCCI branch based on current 8.x:
http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path...
Comment #40
pwolanin CreditAttribution: pwolanin commentedThis is now working in the UI, but the tests are hanging for me locally - not sure why - sometimes I get this after navigating back:
This patch corresponds to what I jsut pushed on the branch linked in #38.
Comment #43
katbailey CreditAttribution: katbailey commentedThanks, @pwolanin. I spent some time today trying to figure out what the architecture of this should be, i.e. taking into account Crell and catch's comments in #34 and #35 respectively. I ended up creating a new branch because I wanted to add the DIC compilation patch and work off that, so that I could use a compiler pass. So I created this branch http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/path....
It's a bit of a monster, totally unfinished and experimental, but I figured I'd leave an update here about it anyway seeing as I'm done for the night and probably won't get back to even thinking about it until the weekend...
Comment #44
katbailey CreditAttribution: katbailey commentedAfter chatting with Crell on irc today I've abandoned the approach mentioned in #43 which required a compiler pass. I pushed a substantial commit to the branch @pwolanin had created (deleted the other branch), which splits apart the responsibilities of the aliasing system and hopefully gets us moving in more or less the right direction. Still too unfinished to put a patch up though I think (for one thing, a bunch of tests will need to get totally rewritten).
Comment #45
katbailey CreditAttribution: katbailey commentedActually, here's a patch, just for kicks...
Comment #47
katbailey CreditAttribution: katbailey commentedHere's a slightly more serious attempt at a patch... (only slightly)
Also, I hate the name PathAliaser - suggestions anyone?
Comment #49
Crell CreditAttribution: Crell commentedWe really really need to eliminate that case by moving more things into the Kernel, after the DIC initializes. *sigh*
ExceptionController should be made ContainerAware, and then it won't need drupal_container() but can access the container directly. I have code in the routing branch that handles ContainerAware controllers that I may split off to a separate patch. Just marking this for reference.
I think at this stage we should go ahead and leave the Aliaser class in core directly; I don't think we need to have a null implementation in core for now.
Are you removing the existing alias code from the other path listener class? There's another subscriber already where this is happening. I don't much care if we spin this off to another subscriber or reuse the same one; that's a performance question as far as I'm concerned, so that should dictate it, but if both are still firing that could be a source of some test fails.
$connection needs to be a defined object property.
With an injected DB connection, this can all be unit tests, not webtests. Yes, it works. :-) If you need a hand setting that up let me know and I can walk you through it.
22 days to next Drupal core point release.
Comment #50
webchickWe're over thresholds on critical tasks, so features in D8 are currently blocked. Any chance this could be downgraded to "major"? Is it actually a release blocker?
Comment #51
webchickAh, according to #28, no. :(
Comment #52
katbailey CreditAttribution: katbailey commentedOK, this should deal with most of Crell's comments in #49.
Yep, and now I'm making it even worse by requiring the database service in the bootstrap container :-( But I have created an issue to deal with this here #1784312: Stop doing so much pre-kernel bootstrapping.
It already is - I just hadn't realized. So I've changed that line to use
$this->container
instead ofdrupal_container()
.I've moved everything back into the Path subsystem (i.e. no longer trying to move it off into path module).
I still haven't devised a decent unit test for this stuff.
I have at least made a start on tackling the problem that makes this issue critical: caching of system paths with a reliable key. In the bundles patch (#1599108: Allow modules to register services and subscriber services (events)), we changed the test at core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php to check that the system path was cached using the alias as the cid, instead of the source. We had to do this because we knew the alias would be used as the cid during cache set because this happened outside of request scope and so a call to current_path() would not be able to return the system path.
To tackle this, I've moved the whole caching of system paths bit inside the AliasManager class. It grabs the system path at the start of a request, then at request close it uses that as the cid when storing the paths to the cache. During lookup it also uses the system path (obviously) as the cid when trying to retrieve from teh cache.
One thing I'm not sure about is whether the AliasManager class itself should maybe implement the EventSubscriberInterface, instead of PathSubscriber handing off to it like this...
Comment #53
katbailey CreditAttribution: katbailey commentedMight as well find out where we're at with tests...
Comment #55
Crell CreditAttribution: Crell commentedDefinitely the subscriber should be separate from AliasManager. AliasManager is an API. PathSubscriber is a bridge to connect two systems. That should not be tightly coupled into a single class.
I'll try to have a look at this soon if I can, but no guarantees. :-( (I have to prep for my next class this weekend.)
Comment #56
katbailey CreditAttribution: katbailey commentedOK, well just to explain why I was thinking that way... if the alias manager is going to take care of caching system paths, it needs to know when to do this, i.e. at request close (and it also needs to be told "here, this is the system path for the current page, use that as your cache key"). So right now I have the PathSubscriber calling startRequest() and endRequest() methods on the AliasManager but it seems totally wrong to have those in the AliasManagerInterface.
Comment #57
Crell CreditAttribution: Crell commentedExceptionController should also get the path manager injected, rather than calling drupal_container().
It may also make the patch easier to reroll if we put the drupal_container()-> call inside the existing functions for now rather than replacing them in all places. Once the actual patch goes in we can then remove the now-just-dumb-wrapper functions in a follow-up.
Comment #58
katbailey CreditAttribution: katbailey commentedTagging for PNW sprint
Comment #59
ksenzeeI worked on this at the PNWDS sprint. There are some genuine test failures that I'm still working on, so I'll assign it to myself in the unlikely case that someone comes by and decides to pick it up in the meantime.
Comment #60
ksenzeeApparently people at BADCamp need work to do so I'll give this up. I have not done any rearchitecting yet; I was hoping to get tests to pass first. They don't yet. Notes:
- This applies to HEAD as of October 30 (881edb14c9dfbf6d986704d19a307ac1041edaac).
- At some point the save() method was changed so that it no longer does a load() to check for the pid. This is breaking tests and actual functionality, and I'm not sure I agree that every call to save() should know whether it's creating a new alias or updating an existing one. We should be able to pass in a source and an alias and let save() take it from there.
- I added hook_path_insert() and hook_path_update() back in. I don't know where they belong, but leaving them out isn't a great option either.
Comment #61
Crell CreditAttribution: Crell commentedNew relevant meta issue: #1830854: [meta] The ESI pipeline battle plan
Comment #62
katbailey CreditAttribution: katbailey commentedOK, I wanted to make some progress on the whole CacheDecorator idea we discussed as a solution to the problem mentioned in #56, plus I needed a break from the bootstrap mess - so here's an initial stab at rearchitecting this to use a CacheDecorator for caching system paths. I am still a bit confused about how this will work when it comes to subrequests.
Comment #64
Crell CreditAttribution: Crell commentedI don't know that we can count on this assumption. There's nothing inherent in this class that says it may only be called from the subscriber; if it breaks if called from elsewhere, then it's still tightly coupled.
Rather, instead we should add a setCacheKey() method and have the subscriber set the cache key after it retrieves it.
It's unclear why we'd want to save the system paths twice. Needs a comment, or rethinking.
This should be called something like path.alias_manager.cached, since it's still really part of the path system.
I don't think we need this to be stealth-hungarian named. :-) Just $aliasManager.
This is a red flag. It's probably a red flag in the existing code, but shoving 3 operations into one method is asking for trouble. For instance, wipe doesn't even belong to this object anymore.
State system should be an injected dependency.
Although, should the whitelisting get moved to the cache object? That way all of the performance-futzing is in one place. Hm. Not sure. I defer to catch.
(And we'll likely be refactoring that when we merge this in with the generator anyway, so maybe we shouldn't stress about it now.)
Needs more complete docblock.
Or, needs to be removed entirely and this logic moved to the caching wrapper.
RETURN_INSERT_ID is the default, I think. No need to set it here.
There's a hook for that??? Why???
If we really need to keep that functionality, switch it to an event listener.
I'm pretty sure this entire class could get converted to a UnitTest, and then eliminate the reference to the container.
Comment #65
sunI don't interface with the insert/update hooks, so those might indeed be questionable (since they do not really provide sufficient context), but I am actively integrating with
hook_path_delete()
in #229568-34: Pathauto in Core (which just moved into #1751210: Convert URL alias form element into a field and field widget), so as to delete a referenced alias in field data values when it is deleted. In general though, I think it is a good idea to keep the low-level API separated, which means that it indeed should invoke CUD hooks. I don't really see why we'd have to move to event listeners here.Comment #66
g.oechsler CreditAttribution: g.oechsler commentedI started hacking a UnitTest exercising CRUD operations on Path objects, basically by cargo culting from PathMatcherTest.
This one applies on top of 1269742-62.alias-manager.patch. I submit it here standalone to avoid getting in the way of katbailey. So it will surely fail on the bot, because of missing Path, AliasManager a.s.o.
Comment #68
g.oechsler CreditAttribution: g.oechsler commentedJust for the fun of it: This one contains a port of LookupTest to a unit test. I just added a method to CrudTest which breaks with the description and naming of it. But hey, as long as I have no feedback whether I'm on the right - er... - path, I won't care too much about wording.
This patch will fail like the one from #66 for the same reason!
Comment #70
catchThe whitelisting, I'm not sure where it lives yet, but it's going to end up looking like #1209226: Avoid slow query for path alias whitelists, not what we have now with the scary query + state. That means the dependencies for the class and some of the structure is going to end up different once that's fixed.
Comment #71
katbailey CreditAttribution: katbailey commentedThis addresses #64, including refactoring the crazy lookup method.
Also, yay g.oechsler :-D Added in those unit tests with some minor changes, and got rid of the existing tests that they replace. Not sure what to do about the url_inbound_alter test - should the url_alter_test module provide its own bundle and register a path subscriber?
Comment #73
Crell CreditAttribution: Crell commentedsun:
If we're going to have CUD events, then yes it should be separated. My statement was one of surprise that such CUD hooks even existed right now in the first place. If we keep them, then they really ought to change to events because events are cleanly injectable and hooks are not. (The same is true of the various entity CUD hooks, too; I haven't raised that question yet because the entity system is still being reshuffled daily, and it is, I think, a December-safe change.)
I looked over the interdiff in #71, and it looks a lot better. I'll have to review the full patch when I'm better rested. :-) Also, the new tests look schweet! :-)
Comment #74
Crell CreditAttribution: Crell commentedWhat's a source lookup?
If the intent here is to pre-lookup a set of aliases, then setSystemPaths() seems like the wrong name. Rather, it's a "load all the aliases!" operation. So it's more like getPathAliases(), or preloadAliases(), or something like that. It's not a set operation at all. That's just a quirk of the way the cache decorator is using it. It's debatable if the cache decorator should even be mentioned here at all.
This whole class still needs docblocks.
Other than that (and the remaining test fails), I like this. We can get rid of those hooks in favor of events later.
Comment #75
g.oechsler CreditAttribution: g.oechsler commentedI tried to track down the bugs that made the tests fail and had a quite interesting tour through many place in core I haven't seen before. After two days I decided to rebase my test environment to latest 8.x dev and ... most of the tests seem to pass now!? Let's check this on the testbot.
Also I found a tiny thing in views_ui.module, so here's a new patch.
Comment #76
g.oechsler CreditAttribution: g.oechsler commentedComment #78
g.oechsler CreditAttribution: g.oechsler commentedI think I figured out what's going wrong, but sadly I have no solution for it.
There are four failing tests, three of them (ForumTest, ImageThemeFunctionTest and JavaScriptTest) pass, if I run them from the web interface of my machine. When I run them via run-tests.sh they fail just like on the bot.
Looks like the problem is in current_path:
When running with run-tests.sh isScopeActive('request') is never true, so we always fall back to _current_path() which produces different/wrong output and therefore failing tests.
Comment #79
BerdirI'd suggest to check if the patch at #1784312: Stop doing so much pre-kernel bootstrapping is fixing this somehow.
Comment #80
katbailey CreditAttribution: katbailey commentedI'll look into this - thanks for pinpointing the problem, g.oechsler!
Comment #81
katbailey CreditAttribution: katbailey commentedComment #82
BerdirThe current standard for these @file docblocks is "Contains..." instead of "Definition of..."
Ah, this is exactly what I suggested in #512026-109: Move $form_state storage from cache to new state/key-value system for KeyValueDatabaseExpirable::garbageCollection(). Would be great if you could comment in there on what would the best way to do this. Not sure where and how we should register such an event. Same for the lock service, which has something like this too (see also me questions about the reliability of such an event in that comment)
Comment #83
katbailey CreditAttribution: katbailey commentedLots of cleanup in this latest patch, should cover #74. I've created a follow-up issue for the CUD hooks, #1844354: Convert path alias CUD hooks to events.
Now, I should also explain some of the changes I made in the previous patch to get the remaining tests to pass:
Comment #84
disasm CreditAttribution: disasm commentedI like the overall patch, and think it's much better than what we had before, but since you asked for nitpicky, I have two simple comments:
When you're doing math for calculating times, it's nice to have a comment like expire time of 24 hours.
lowercase string
Comment #85
Crell CreditAttribution: Crell commentedThis makes me sad. :-( Why do we have to call out to a legacy function here? More specifically... in what situation should $path be an optional parameter? I can't see how this use case even exists, and if it does, I still hope there's a cleaner way than this.
As above, I don't think this is a legitimate use case. If you want the current path, there are other ways to get it and then you can call getPathAlias() with an actual path. In 7 years of Drupaling I cannot recall a time when I wanted "get me the current aliased path"... or at least not one that I didn't know at the time was an ugly hack of evil in the theme layer that I felt sick doing. :-)
I think that's all I could find left. If we can resolve that, we can finally get it RTBC. woo!
Comment #86
katbailey CreditAttribution: katbailey commentedComment #87
Crell CreditAttribution: Crell commentedI think... it is time.
Comment #88
Dries CreditAttribution: Dries commentedHow about a helper function like
drupal_service()
:$normal_path = drupal_container()->get('path.alias_manager')->getSystemPath($path);
Would become:
drupal_service('path.alias_manager')->getSystemPath('foo');
Obviously, that would be tackled in another issue but I think it would improve DX.
Comment #89
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #90
Dave ReidI don't see anymore how hook_url_inbound_alter() is getting invoked since drupal_get_normal_path() was removed, but the hook is still defined by system.api.php?
We could also use change records for how to replace all the removed functions? And possibly hook_url_inbound_alter() if the above is true?
I've also noticed that this removes the flexibility to pass additional information in along with the path aliases. In Drupal 7, we can do something like this:
But now all the arguments are hard-coded:
This seems like a regression which I know will affect a few contrib modules. Can the crud methods accept an array like it did with Drupal 7, rather than making it more of a DX regression?
Comment #91
katbailey CreditAttribution: katbailey commentedYes, hook_url_inbound_alter needs to be removed - I'll try and get to this tonight and write a change notice, including the new way to alter the inbound path.
Not sure what to say about the path_save() issue though... I mean, I wasn't aware that there was code that took advantage of something like that and it seems to me like an odd way to do things. If I could understand the use case a bit better I might be able to help figure out how to make it work with the new set-up.
Comment #92
katbailey CreditAttribution: katbailey commentedOops
Comment #93
Dave ReidFor example, it's used by Pathauto to indicate that this alias is an automatically generated alias and not a manual user-created alias. We also add in $path['original'] to show what the original alias was if it is being changed or updated, so that the Redirect module can automatically create redirects so that the original alias redirects to the new alias.
http://drupalcode.org/project/pathauto.git/blob/refs/heads/7.x-1.x:/path...
http://drupalcode.org/project/redirect.git/blob/refs/heads/7.x-1.x:/redi...
Comment #94
Dave ReidI'm just not sure why path_save($path) was converted to essentially path_save($source, $alias, $language) when it would have been an easier conversion to leave it as an array. I guess it seemed to originate from:
But I would definitely consider url aliases their own data objects since we have path_load(). I'm not sure this change was justified.
Comment #95
catchWhether they're data objects or not is really a discussion for #1553358: Figure out a new implementation for url aliases but I'm also not sure why it was changed here.
Comment #96
sun@Dave Reid's argumentation and reasonings make sense to me. And he's definitely one of the most knowledgeable people in this field, especially when it comes to what contrib needs and is doing.
Changing passed/accepted arguments sounds like a quick fix though? Let's quickly adjust that.
Comment #97
sunI'm confronted with these changes in #1751210: Convert URL alias form element into a field and field widget, and indeed, the new arguments to
save()
anddelete()
are non-trivial to work with. I would have expected to get the $pid as only, scalar return value, or as @Dave Reid says, accept an array as input, take it by reference, and populate 'pid' accordingly, as in the old code.However, the following is actually much more critical:
This hard-codes the Database implementation of the key/value service, which is an absolute no-go, since that ignores any possible service overrides, and consequently breaks all DrupalUnitTestBase tests.
Given that this part is effectively holding up aforementioned issue, which is bound to feature freeze, I'd appreciate if we could roll this back for the time being.
Comment #98
chx CreditAttribution: chx commentedThere are a few problems here but none of them seem to be worth a rollback. Once #1784312: Stop doing so much pre-kernel bootstrapping is in we can change keyvalue.database to keyvalue easily. That will immediately fix one of the problems, I simply can't care enough to fix it outside of that patch especially as I mentioned in that issue I had severe problems using DrupalUnitTestBase without it so trying to fix DUTB before that one I consider a futile exercise. The current mess of drupal_container() is not something I want to touch since I have such a beautiful fix over there in TestBundle.
The other one that Dave Reid raised is a valid point. Addressing it, however, while keeping katbailey's idea of meaningful parameters for IDE integration, better docs and all that is not particularly hard, just allow for passing in arbitrary additional things if you need them. Also, as I do not see at first sight what, if anything is using the return value of save, I just changed it to return the $pid.
Comment #100
chx CreditAttribution: chx commented#98: 1269742_98.patch queued for re-testing.
Note: the fail was, Setup environment: failed to clear checkout directory. Bot fluke.
Comment #101
sunWell, the other thing that's wonky to work with is the $pid argument.
For an existing path alias, the $pid should always be passed, and technically it should even come first, because it's the most important argument for an existing path alias.
I think that path aliases are indeed mini data objects - borderline entities even, although I wouldn't necessarily convert them into a true entity system entity.
If typed arguments are the issue, then we should have converted the array into a PathAlias object.
So, yeah, like others, I also don't really understand why this was changed as part of this patch.
Comment #102
katbailey CreditAttribution: katbailey commentedAdded a follow-up for the keyvalue thing #1848072: Path alias manager hardcodes keyvalue.database instead of using the generic factory
Comment #103
Crell CreditAttribution: Crell commentedGiven the length of this thread and that it's a critical, can we close it and move "clean up Path CRUD API" to a new issue? There's probably plenty of cleanup to be done there, and we already have one follow-up for that to shift the hook calls to events.
Comment #104
catch@Crell I think that's #1553358: Figure out a new implementation for url aliases, I've bumped it to major. I could be persuaded it's critical since it blocks deployment of path aliases and the (unintentional) API usage regression here.
And yeah let's please move that discussion there - that issue stalled some time ago and if we'd persisted there we'd not be having this conversation now.
Comment #105
sunI'm not 100% sure yet (as it's not exactly easy to follow the new code flow), but I suspect that this patch additionally resulted in the path alias cache getting cleared/refreshed too late — i.e.,
When entering a URL alias in the node edit form and saving it, you are redirected to the non-aliased path (e.g., node/1). Only on that second request, when hovering over the default "View" tab of the shown node, the new alias is known, and clicking the tab gets you to the aliased node path.
Previously, the new URL alias took effect immediately; i.e., adjusting the redirect URL of the form submission already.
Comment #106
Crell CreditAttribution: Crell commentedsun: Please file that as a new issue with a patch and let's tackle that there.
This issue still needs a change notice, so not closing yet but anything that's not a change notice please file as a follow-up issue.
Comment #107
marcingy CreditAttribution: marcingy commentedComment #108
sunThanks, but that's not exactly the way I understand collaborative improvements — if my changes break stuff or have unintended consequences, then I understand it as my responsibility to ensure that the system stays functional for others and fix it accordingly, so I'll leave that to you folks. If that differs from your understanding, then I can't help. It took me long enough to debug this change and work around it for other issues already.
Comment #109
bforchhammer CreditAttribution: bforchhammer commentedEntity translation code still has a call to
path_delete()
which breaks the ability to delete translations: #1852394: Fatal error: Call to undefined function path_delete().A change notice with instructions on how to fix it would be appreciated :)
Comment #110
katbailey CreditAttribution: katbailey commentedAdded a change notice: [#1853148]. Not sure what the status of this issue should be now :-/
Comment #111
aspilicious CreditAttribution: aspilicious commentedThe change notice is prety clear and easy to understand. As all the remaining problems are moved to other issues I'm going to mark this fixed. Please reopen if I misunderstood some comments.
Thnx :)
Comment #112
Dave ReidI do not consider the issue of path aliases are no longer passed as arrays solved by #1553358: Figure out a new implementation for url aliases which is a much larger architectural decision. Filed #1854284: Path aliases are no longer arrays and cannot pass additional data to path hooks as a major bug report.
Comment #114
effulgentsia CreditAttribution: effulgentsia commented#1875086: Improve DX of drupal_container()->get()
Comment #115
tim.plunkettThis issue is still referenced by current_path(). What is the new issue for that?
Comment #116
amateescu CreditAttribution: amateescu commentedWondering about the same thing as #115.
Comment #116.0
amateescu CreditAttribution: amateescu commentedUpdated issue summary.
Comment #117
yched CreditAttribution: yched commentedBump on #115
:-)
Comment #118
sunCreated #2237001: Remove no longer needed _current_path() fallback for early bootstrap for that stale @todo