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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm’s picture

Status: Active » Needs review
FileSize
5.78 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2209145_1.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
407 bytes
5.77 KB
chx’s picture

Status: Needs review » Needs work

+ $query = db_select('url_alias')

You mean

+ $query = $this->connection->select('url_alias')

slashrsm’s picture

Status: Needs work » Needs review
FileSize
5.81 KB
1.56 KB

Ah.... yes.

slashrsm’s picture

FileSize
119.67 KB
4.92 KB

Added another query.

slashrsm’s picture

FileSize
10.42 KB

This should be the right patch.

chx’s picture

Assigned: slashrsm » catch
Status: Needs review » Reviewed & tested by the community

As 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.

slashrsm’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -246,4 +246,87 @@ public function lookupPathSource($path, $langcode) {
    +   * Checks if same alias already exists for a different path.
    

    The word "same" is redundant.

  2. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -246,4 +246,87 @@ public function lookupPathSource($path, $langcode) {
    +   * Loads list of alias for one page of alias admin listing.
    

    How about "Loads aliases for admin listing."

  3. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -246,4 +246,87 @@ public function lookupPathSource($path, $langcode) {
    +   * Check if path (prefix) already has any alias-es.
    +   *
    +   * @param $path_prefix
    +   *   Path prefix to test for.
    +   * @return
    +   *   TRUE if alias-es exists, FALSE otherwise.
    +   */
    +  public function pathHasAlias($path_prefix) {
    +    $query = $this->connection->select('url_alias', 'u');
    +    $query->addExpression(1);
    +    return (bool) $query
    +      ->condition('u.source', $this->connection->escapeLike($path_prefix) . '%', 'LIKE')
    +      ->range(0, 1)
    +      ->execute()
    +      ->fetchField();
    +  }
    

    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?

fgm’s picture

Status: Needs work » Needs review
FileSize
11.11 KB

How about that version ? It seems rather closer to what the code does..

Status: Needs review » Needs work
slashrsm’s picture

Status: Needs work » Needs review
FileSize
10.42 KB
552 bytes

@fgm: please upload interdiff along the main patch next time: https://drupal.org/documentation/git/interdiff

fgm’s picture

Rerolled with an interdiff on #7: I had missed a method call.

fgm’s picture

The related interface issue appears to already exist as #2199381: Add \Drupal\Core\Path\PathInterface

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@fgm
Please use the -up format for the interdiff, otherwise it is quite hard to read.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -246,4 +246,87 @@ public function lookupPathSource($path, $langcode) {
    +   * Checks if alias already exists for a different path.
    

    This 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?

  2. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -246,4 +246,87 @@ public function lookupPathSource($path, $langcode) {
    +   * Check if any alias exists starting by $path_prefix.
    

    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?

fgm’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
11.14 KB

OK, how about this wording ?

slashrsm’s picture

FileSize
10.59 KB
2.81 KB

Let's see what happens if we remove $pid....

fgm’s picture

Followup issue after #2199381: Add \Drupal\Core\Path\PathInterface if merged: #2224933: Path storage abstraction followup. No longer needed after next patch.

slashrsm’s picture

FileSize
10.6 KB
965 bytes

#2199381: Add \Drupal\Core\Path\PathInterface has landed. We can typehint PathInterface now.

fgm’s picture

21: 2209145_21.patch queued for re-testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -186,4 +186,83 @@ public function lookupPathSource($path, $langcode) {
    +  public function adminListing($header, $keys = NULL) {
    

    I wonder whether some better describing name like :getAliasesForAdminListing could be used. Otherwise this could be nearly a render array.

  2. +++ b/core/lib/Drupal/Core/Path/Path.php
    @@ -186,4 +186,83 @@ public function lookupPathSource($path, $langcode) {
    +*@return
    

    ups

fgm’s picture

FileSize
10.69 KB
1.25 KB

Rerolled.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Seems like all points are addressed. https://drupal.org/documentation/git/interdiff thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2209145_24.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.72 KB
chx’s picture

Status: Needs review » Reviewed & tested by the community
slashrsm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.01 KB

Status: Needs review » Needs work

The last submitted patch, 29: 2209145_29.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
11.98 KB

Meh....

Status: Needs review » Needs work

The last submitted patch, 31: 2209145_31.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
12.06 KB

Now it should really install. It installs via both drush and UI locally + all PHPUnit tests pass.

Status: Needs review » Needs work

The last submitted patch, 33: 2209145_33.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
11.98 KB
905 bytes

Status: Needs review » Needs work

The last submitted patch, 35: 2209145_35.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
11.85 KB
1.72 KB
chx’s picture

Status: Needs review » Reviewed & tested by the community

Let's try this again.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

  • Commit ac120e2 on 8.x by catch:
    Issue #2209145 by slashrsm, fgm, chx: Move all path alias SQL queries to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.