Updated: Comment #0
Problem/Motivation
We already moved most of DB queries to Path CRUD service (#2136503: Make \Drupal\Core\Path\AliasManager storage independent), but there are still some SQL queries into url_alias database left. We should concentrate all queries in one place to make storage swap-ability possible.
Proposed resolution
Move path alias SQL queries that currently live in path.module and path.admin.inc to Path CRUD service (\Drupal\Core\Path\Path).
Remaining tasks
- prepare patch
User interface changes
N/A.
API changes
N/A.
Comment | File | Size | Author |
---|---|---|---|
#37 | interdiff.txt | 1.72 KB | slashrsm |
#37 | 2209145_37.patch | 11.85 KB | slashrsm |
#35 | interdiff.txt | 905 bytes | slashrsm |
#35 | 2209145_35.patch | 11.98 KB | slashrsm |
#33 | 2209145_33.patch | 12.06 KB | slashrsm |
Comments
Comment #1
slashrsm CreditAttribution: slashrsm commentedComment #3
slashrsm CreditAttribution: slashrsm commentedComment #4
chx CreditAttribution: chx commented+ $query = db_select('url_alias')
You mean
+ $query = $this->connection->select('url_alias')
Comment #5
slashrsm CreditAttribution: slashrsm commentedAh.... yes.
Comment #6
slashrsm CreditAttribution: slashrsm commentedAdded another query.
Comment #7
slashrsm CreditAttribution: slashrsm commentedThis should be the right patch.
Comment #8
chx CreditAttribution: chx commentedAs catch is listed in MAINTAINERS as the sole Path system maintainer I have no better idea than marking this ready and assigning it to him.
Comment #9
slashrsm CreditAttribution: slashrsm commentedComment #10
alexpottThe word "same" is redundant.
How about "Loads aliases for admin listing."
This needs improving but I haven't come up with something yet. I don't feel the method has the right name.
Also I notice that Path does not have an interface so swapping out implementation will prove difficult until it does - can someone open a followup to create one?
Comment #11
fgmHow about that version ? It seems rather closer to what the code does..
Comment #13
slashrsm CreditAttribution: slashrsm commented@fgm: please upload interdiff along the main patch next time: https://drupal.org/documentation/git/interdiff
Comment #14
fgmRerolled with an interdiff on #7: I had missed a method call.
Comment #15
fgmThe related interface issue appears to already exist as #2199381: Add \Drupal\Core\Path\PathInterface
Comment #16
dawehner@fgm
Please use the -up format for the interdiff, otherwise it is quite hard to read.
Comment #17
catchThis can check for any path if $source is NULL, could do with a more generic summary, maybe just remove 'for a different path'?
Also what's the use case for passing in $pid?
Should be 'starting with'. Also not sure about the use of $path_prefix here, we already use that for things like language prefixes for things that precede the actual alias. i.e. the $prefix option for UrlGenerator::generateFromPath. Not sure what, $pattern? $substring?
Comment #18
fgmOK, how about this wording ?
Comment #19
slashrsm CreditAttribution: slashrsm commentedLet's see what happens if we remove $pid....
Comment #20
fgmFollowup issue after #2199381: Add \Drupal\Core\Path\PathInterface if merged: #2224933: Path storage abstraction followup.No longer needed after next patch.Comment #21
slashrsm CreditAttribution: slashrsm commented#2199381: Add \Drupal\Core\Path\PathInterface has landed. We can typehint PathInterface now.
Comment #22
fgm21: 2209145_21.patch queued for re-testing.
Comment #23
dawehnerI wonder whether some better describing name like :getAliasesForAdminListing could be used. Otherwise this could be nearly a render array.
ups
Comment #24
fgmRerolled.
Comment #25
chx CreditAttribution: chx commentedSeems like all points are addressed. https://drupal.org/documentation/git/interdiff thanks!
Comment #27
chx CreditAttribution: chx commentedRerolling after #2221065: Rename default 'cache' cache bin to 'default'.
Comment #28
chx CreditAttribution: chx commentedComment #29
slashrsm CreditAttribution: slashrsm commentedReroll after #2208631: Rename \Drupal\Core\Path\Path to \Drupal\Core\Path\AliasStorage.
Comment #31
slashrsm CreditAttribution: slashrsm commentedMeh....
Comment #33
slashrsm CreditAttribution: slashrsm commentedNow it should really install. It installs via both drush and UI locally + all PHPUnit tests pass.
Comment #35
slashrsm CreditAttribution: slashrsm commentedComment #37
slashrsm CreditAttribution: slashrsm commentedComment #38
chx CreditAttribution: chx commentedLet's try this again.
Comment #39
catchThanks!