Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#98 interdiff-1987802-94-98.txt1.86 KBfgm
#98 1987802-98.patch32.45 KBfgm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,856 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#89 interdiff.txt6.93 KBpfrenssen
#89 1987802-path-controllers-89.patch32.88 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1987802-path-controllers-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#88 1987802-path-controllers-88.patch32.86 KBpfrenssen
#86 1987802-path-controllers-86.patch32.86 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,701 pass(es).
[ View ]
#86 1987802-diff-83-86.txt1.09 KBvijaycs85
#83 1987802-path-controllers-83.patch32.83 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 59,833 pass(es), 2 fail(s), and 8 exception(s).
[ View ]
#83 1987802-diff-74-83.txt3.76 KBvijaycs85
#74 1987802-path-controllers-74.patch33.16 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,265 pass(es).
[ View ]
#72 drupal_1987802_72.patch33.31 KBXano
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#69 path-1987802-69.patch33.16 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,223 pass(es).
[ View ]
#69 interdiff.txt608 bytestim.plunkett
#65 drupal8.path-module.1987802-65.patch33.85 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]
#65 interdiff.txt2.69 KBpfrenssen
#62 drupal8.path-module.1987802-62.patch33.24 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,570 pass(es).
[ View ]
#62 interdiff.txt730 bytespfrenssen
#61 drupal8.path-module.1987802-61.patch32.52 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#61 interdiff.txt4.99 KBpfrenssen
#58 interdiff.txt730 bytesdawehner
#56 drupal8.path-module.1987802-56.patch32.76 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,649 pass(es).
[ View ]
#56 interdiff.txt4.09 KBpfrenssen
#54 drupal8.path-module.1987802-54.patch32.17 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 57,073 pass(es), 105 fail(s), and 86 exception(s).
[ View ]
#50 drupal8.path-module.1987802-50.patch31.83 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.path-module.1987802-50.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#50 interdiff.txt3.03 KBdisasm
#46 drupal8.path-module.1987802-46.patch31.82 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,087 pass(es).
[ View ]
#46 interdiff.txt3.21 KBdisasm
#43 path-1987802-43.patch32.21 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,043 pass(es).
[ View ]
#43 interdiff.txt1.67 KBplopesc
#40 path-1987802-40.patch32.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,353 pass(es).
[ View ]
#40 interdiff.txt4.38 KBtim.plunkett
#38 drupal8.path-module.1987802-38.patch32.05 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,473 pass(es), 37 fail(s), and 10 exception(s).
[ View ]
#38 interdiff.txt8.33 KBdisasm
#35 drupal8.path-module.1987802-35.patch31.51 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,368 pass(es), 37 fail(s), and 10 exception(s).
[ View ]
#35 interdiff.txt1.4 KBdisasm
#34 drupal8.path-module.1987802-34.patch31.53 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,913 pass(es), 37 fail(s), and 20 exception(s).
[ View ]
#34 interdiff.txt3.29 KBdisasm
#32 drupal8.path-module.1987802-32.patch31.16 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,430 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#32 interdiff.txt1.06 KBdisasm
#31 drupal8.path-module.1987802-31.patch31.14 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#31 interdiff.txt6.71 KBdisasm
#29 drupal8.path-module.1987802-29.patch30.6 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,266 pass(es).
[ View ]
#29 interdiff.txt6.24 KBdisasm
#27 drupal8.path-module.1987802-27.patch30.75 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,314 pass(es), 37 fail(s), and 10 exception(s).
[ View ]
#27 interdiff.txt5.29 KBdisasm
#24 drupal8.path-module.1987802-24.patch30.08 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/path/lib/Drupal/path/Controller/PathController.php.
[ View ]
#24 interdiff.txt13.03 KBdisasm
#20 drupal-1987802-20.patch31.17 KBlaurentchardin
PASSED: [[SimpleTest]]: [MySQL] 55,971 pass(es).
[ View ]
#15 drupal-1987802-15.patch16.13 KBlaurentchardin
PASSED: [[SimpleTest]]: [MySQL] 55,896 pass(es).
[ View ]
#12 drupal-1987802-12.patch16.19 KBlaurentchardin
PASSED: [[SimpleTest]]: [MySQL] 55,959 pass(es).
[ View ]
#9 drupal-1987802-9.patch17.15 KBlaurentchardin
FAILED: [[SimpleTest]]: [MySQL] 57,348 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#8 drupal-1987802-8.patch15.34 KBlaurentchardin
PASSED: [[SimpleTest]]: [MySQL] 56,702 pass(es).
[ View ]
#5 drupal-1987802-5.patch8.64 KBlaurentchardin
FAILED: [[SimpleTest]]: [MySQL] 55,837 pass(es), 3 fail(s), and 28 exception(s).
[ View ]
#2 drupal-convert_path_admin_overview-1987802-2.patch8.61 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 55,611 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Comments

Assigned:Unassigned» h3rj4n

StatusFileSize
new8.61 KB
FAILED: [[SimpleTest]]: [MySQL] 55,611 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

I think this should work, dunno what I'm doing wrong.

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-convert_path_admin_overview-1987802-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.64 KB
FAILED: [[SimpleTest]]: [MySQL] 55,837 pass(es), 3 fail(s), and 28 exception(s).
[ View ]

Fixing namespace + return static on ControllerInterface::create()

Status:Needs review» Needs work

The last submitted patch, drupal-1987802-5.patch, failed testing.

Just some general comments which could be improved.

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,114 @@
+  public function adminOverview() {

Just to be sure to include the @return on the next rerole.

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,114 @@
+    $alias_exists = (bool) db_query_range('SELECT 1 FROM {url_alias} WHERE langcode <> :langcode', 0, 1, array(':langcode' => LANGUAGE_NOT_SPECIFIED))->fetchField();
...
+    $query = db_select('url_alias')

This could use an injected database connection

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,114 @@
+      $row['data']['alias'] = l(truncate_utf8($data->alias, 50, FALSE, TRUE), $data->source, array(
...
+      $row['data']['source'] = l(truncate_utf8($data->source, 50, FALSE, TRUE), $data->source, array(

truncate_utf8 could use the Unicode class

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,114 @@
+      if ($data->alias != drupal_container()->get('path.alias_manager')->getPathAlias($data->source, $data->langcode)) {

So what about inject the alias manager :)

+++ b/core/modules/path/path.moduleundefined
@@ -59,10 +59,7 @@ function path_menu() {
-    'weight' => -5,

Please don't kill the weight.

Status:Needs work» Needs review
StatusFileSize
new15.34 KB
PASSED: [[SimpleTest]]: [MySQL] 56,702 pass(es).
[ View ]

Here is a more revamped patch.

Updated:

  1. Moved the filtering form to a FormInterface class
  2. Removed the 'locale' module dependency since it is not used, and triggers a *batch* download of the po files, and somewhat manage to kill the simpletest process (more than 250 recursive calls, mostly due to the batch ui redirects).
  3. Implemented most of feedbacks
  4. Not sure about DI'ing : path.alias_manager vs path.alias_manager.cached ?

New:

  1. I propose to store the search keyword in session, as it is done by the dblog module, since passing the value in the url does not seem very clean.

Issues:

  1. there is an issue with 'admin/config/search/path/list' as MENU_DEFAULT_LOCAL_TASK : the tab does not show in the UI anymore, since i moved the parent to the new routing system. Might be related to #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK
  2. I still have a simpletest task not passing, but i don't think this is related to the current patch. There are some weirdness with the cache() object here (due to latest changes?).
    $whitelist = cache()->get('path_alias_whitelist'); does not seem to return anything, making the testPathCache() fails. I could use some help here.

StatusFileSize
new17.15 KB
FAILED: [[SimpleTest]]: [MySQL] 57,348 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolling patch.

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,171 @@
+   * @var \Drupal\Core\Path\AliasManager
...
+   * @param AliasManager $alias_manager
...
+  public function __construct(Connection $database, ModuleHandlerInterface $module_handler, AliasManager $alias_manager) {

Alias manager have an interface so let's use it

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,171 @@
+   * @param Connection $database
...
+   * @param ModuleHandlerInterface $module_handler

This docs should have the full namespace added at the front.

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,171 @@
+    // TODO any service to DIC for accessing the session ?

See #1858196: [meta] Leverage Symfony Session components so let's adapt the $_SESSION

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,171 @@
+    $query = db_select('url_alias')

Should also use $this->database

+++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.phpundefined
@@ -0,0 +1,87 @@
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
+   *
+   * @param array $form
+   *   An associative array containing the structure of the form.
+   * @param array $form_state
+   *   An associative array containing the current state of the form.

This bits can be replace by {@inheritdoc}

+++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.phpundefined
@@ -0,0 +1,87 @@
+   * { @inherits }
...
+   * { @inherits }

The exact pattern in {@inheritdoc}

+++ b/core/modules/path/path.moduleundefined
@@ -60,15 +60,20 @@ function path_menu() {
+  $items['admin/config/search/path/add'] = array(
+    'title' => 'Add alias',
+    'page callback' => 'path_admin_edit',
+    'access arguments' => array('administer url aliases'),
+    'type' => MENU_LOCAL_ACTION,
+    'file' => 'path.admin.inc',
+  );
   $items['admin/config/search/path/edit/%path'] = array(
     'title' => 'Edit alias',
     'page callback' => 'path_admin_edit',
@@ -80,13 +85,6 @@ function path_menu() {
@@ -80,13 +85,6 @@ function path_menu() {
     'title' => 'Delete alias',
     'route_name' => 'path_delete',
   );
-  $items['admin/config/search/path/add'] = array(
-    'title' => 'Add alias',
-    'page callback' => 'path_admin_edit',
-    'access arguments' => array('administer url aliases'),
-    'type' => MENU_LOCAL_ACTION,
-    'file' => 'path.admin.inc',

There is no reason to move this bit around.

Status:Needs review» Needs work

The last submitted patch, drupal-1987802-9.patch, failed testing.

StatusFileSize
new16.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,959 pass(es).
[ View ]

Rerolling patch.

Status:Needs work» Needs review

Forgot to change the status.

Assigned:h3rj4n» Unassigned

Manual testing of the page works fine.

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.phpundefined
@@ -0,0 +1,171 @@
+ * Controller routines for book routes.
...
+class PathController implements ControllerInterface {

Let's talk about path :)

+++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.phpundefined
@@ -0,0 +1,79 @@
\ No newline at end of file

Please add a linebreak at the end of the file.

StatusFileSize
new16.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,896 pass(es).
[ View ]

Damn... nice catch dawehner !
Here's an update.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, drupal-1987802-15.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

#15: drupal-1987802-15.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

There we go

Status:Reviewed & tested by the community» Needs work

Changing state because we need to merge this one with #1987800: Convert path_admin_edit() to a new style controller

With the support of MENU_LOCAL_TASK, introduced by #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu(), those 2 issues are interdependent.
Also see https://drupal.org/node/2007444

We need the path_admin_overview route to support the add button local task for the path_admin_edit one.

I proposed on IRC to merge both and work on an unique patch, as it will make review and testing easier.

Assigned:Unassigned» laurentchardin
Status:Needs work» Needs review
Issue tags:+MENU_LOCAL_ACTION
StatusFileSize
new31.17 KB
PASSED: [[SimpleTest]]: [MySQL] 55,971 pass(es).
[ View ]

Here we are.

Summing-up:

  • Rearrange routing.yml to make it more logical
  • All hook_menu migrated to the new routing system
  • Implemented hook_local_actions
  • Removed now useless path.admin.inc since logic have been moved into forms

Tests:

  • Removed locale dependency since it's not really used (and also make the simpletest fails due to batch UI requests)

Feebacks needed:

  • My english is not that good, i think the phpdoc/comments might need some rephrasing
  • Please also check some of the variable names, since there may be better candidate (well.. naming...)
  • Speaking of DI, do i need to inject path.alias_manager or path.alias_manager.cached ?
  • What about "MENU_DEFAULT_LOCAL_TASK" anymore ?

** Updated ** :

Regarding the service: use the cached version of it.

Status:Needs review» Needs work

@@ -0,0 +1,228 @@
+    $query = $this->database->select('url_alias', 'u')
+      ->fields('u', array('pid'))
+      ->where('pid <> :pid AND alias = :alias AND langcode = :langcode', array(
+        ':pid' => $pid,
+        ':alias' => $alias,
+        ':langcode' => $langcode))
+      ->range(0,1);
+    $alias_exists = (bool) $query->execute()->fetchField();

This should be moved to a new method on the Path service. There's no good reason for SQL in a form, certainly not in the validate method.

Status:Needs work» Needs review
StatusFileSize
new13.03 KB
new30.08 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/path/lib/Drupal/path/Controller/PathController.php.
[ View ]

cleanup of controllers and forms.
Moved query to aliasManager service.

Status:Needs review» Needs work

The last submitted patch, drupal8.path-module.1987802-24.patch, failed testing.

  1. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -312,4 +312,20 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
    +  /**
    +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,151 @@
    +<?php
    +/**

    Missing blank line

  2. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,151 @@
    +

    There is a moduleHandler() method on ControllerBase

  3. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,151 @@
    +    $this->connection = $this->container->get('database');
    +    $this->moduleHandler = $this->container->get('module_handler'),
    +    $this->aliasManager = $this->container->get('path.alias_manager')

    Does this even work? setContainer() isn't called until afterward I beleive.

  4. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,151 @@
    +   * The admin overview

    Missing .

  5. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,151 @@
    +   *  Returns a listing of all defined URL aliases.

    Bad indentation

  6. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,189 @@
    +
    +  /**
    +   * Constructs a \Drupal\Core\Form\FormBase object.
    +   *
    +   */
    +  public function __construct() {
    +    $this->path = $this->container->get('path.crud');
    +    $this->moduleHandler = $this->container->get('module_handler');
    +    $this->aliasManager = $this->container->get('path.alias.manager');
    +  }

    This definitely doesn't work... FormBase implements ControllerInterface, use that.

  7. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,189 @@
    +    switch ($op) {
    +      case $this->t('Save'):
    +        $this->path->save($source, $alias, $langcode, $pid);
    +        drupal_set_message(t('The alias has been saved.'));
    +        $form_state['redirect'] = 'admin/config/search/path';
    +        break;
    +      case $this->t('Delete'):
    +        $destination = array();
    +        if (isset($_GET['destination'])) {
    +          $destination = drupal_get_destination();
    +          unset($_GET['destination']);
    +        }
    +        $form_state['redirect'] = array('admin/config/search/path/delete/' . $pid, array('query' => $destination));
    +        break;
    +++ /dev/null
    @@ -1,321 +0,0 @@
    -function path_admin_form_delete_submit($form, &$form_state) {
    -  $destination = array();
    -  $query = Drupal::request()->query;
    -  if ($query->has('destination')) {
    -    $destination = drupal_get_destination();
    -    $query->remove('destination');
    -  }
    -  $form_state['redirect'] = array('admin/config/search/path/delete/' . $form_state['values']['pid'], array('query' => $destination));
    ...
    -function path_admin_form_submit($form, &$form_state) {
    -  // Remove unnecessary values.
    -  form_state_values_clean($form_state);

    Please split this back into two methods, this is really confusing and hacky.

  8. +++ b/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php
    @@ -7,6 +7,9 @@
    +use Drupal\Core\Path\AliasWhitelist;
    +++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageUiTest.php
    +++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageUiTest.php
    @@ -17,7 +17,7 @@ class PathLanguageUiTest extends PathTestBase {

    ?

  9. +++ b/core/modules/path/path.module
    @@ -58,38 +58,47 @@ function path_menu() {
    +  // @todo add a paramconverter for path ?

    These @todos should be proper sentences (here and throughout the patch)

  10. +++ b/core/modules/path/path.module
    @@ -58,38 +58,47 @@ function path_menu() {
    + * Implements hook_local_action()
    + */
    +function path_local_actions() {
    +  return array(
    +    array(
    +      'route_name' => 'path_add',
    +      'title' => t('Add alias'),
    +      'appears_on' => array(
    +        'path_overview',
    +      ),
    +    ),
    +  );
    +}

    This doesn't work anymore. It needs to be a YAML file

Status:Needs work» Needs review
StatusFileSize
new5.29 KB
new30.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,314 pass(es), 37 fail(s), and 10 exception(s).
[ View ]

Addressing most of #26.

Status:Needs review» Needs work

The last submitted patch, drupal8.path-module.1987802-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.24 KB
new30.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,266 pass(es).
[ View ]

accidentally amended my commit, so interdiff is to #24. This should resolve all the test failures. I've manually tested it, and all is functioning.

Status:Needs review» Needs work
Issue tags:+FormBase
  1. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -312,4 +312,20 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
    +  /**
    +   * Checks if a path alias exists.
    +   *
    +   * @return bool
    +   */
    +  public function checkAliasExists($pid, $alias, $langcode) {

    Doc block needs update.

  2. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -312,4 +312,20 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
    +    $query = $this->connection->select('url_alias', 'u')

    We are not adding any tags why not query instead of select.

  3. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,155 @@
    +  public function adminOverview($alias = NULL) {

    @peram missing in doc block.

  4. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,155 @@
    +    $query = $this->connection->select('url_alias', 'u')
    +      ->fields('u', array('pid'))
    +      ->where('langcode <> :langcode', array(':langcode' => Language::LANGCODE_NOT_SPECIFIED))
    +      ->range(0,1);

    We can use query here?

  5. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,155 @@
    +      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
    +      ->extend('Drupal\Core\Database\Query\TableSortExtender');

    I think we have to add \Drupal.
    Maybe we can add use statement on top.

  6. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,155 @@
    +      $row['data']['alias'] = l(Unicode::truncate($data->alias, 50, FALSE, TRUE), $data->source, array(
    ...
    +      $row['data']['source'] = l(Unicode::truncate($data->source, 50, FALSE, TRUE), $data->source, array(

    We can replace l.

  7. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,200 @@
    +   * {@inheritdoc}
    +   * @param int $pid
    +   *   an alias id for editing or NULL to add a new one.
    +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,79 @@
    +   * {@inheritdoc}
    +   * @param string $alias
    +   *   A string containing a keyword for searching aliases.

    We can't mix these two.

  8. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,200 @@
    +    drupal_set_title($pathAlias['alias']);

    hmmm

  9. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,200 @@
    +      '#field_prefix' => url(NULL, array('absolute' => TRUE)),
    ...
    +      '#field_prefix' => url(NULL, array('absolute' => TRUE)),

    Injection

  10. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,200 @@
    +      // @todo add DI for language_list

    issue link maybe or we can remove this line.

  11. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,79 @@
    +class PathAdminFilterForm implements FormInterface {

    FormBase

  12. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,79 @@
    +      '#title' => t('Filter aliases'),
    ...
    +      '#value' => t('Filter'),
    ...
    +        '#value' => t('Reset'),
    ...
    +      form_set_error('type', t('You must provide a keyword something to filter by.'));

    $this->t.

Status:Needs work» Needs review
StatusFileSize
new6.71 KB
new31.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Handling most of issues in #30. #2 and #4, lets handle in a follow-up (trying to minimize the refactor). #6, what does l() become in a class? I don't see an equivalent method on UrlGeneratorInterface. #9 lets handle in a follow-up.

StatusFileSize
new1.06 KB
new31.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,430 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

$this->l() is part of ControllerBase. Fixing.

Why we need follow-up other then these minor issues?

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -315,7 +315,15 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
+   * #param string $alias

Typo

StatusFileSize
new3.29 KB
new31.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,913 pass(es), 37 fail(s), and 20 exception(s).
[ View ]

Fixing typo, adding urlGenerator to EditForm.

StatusFileSize
new1.4 KB
new31.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,368 pass(es), 37 fail(s), and 10 exception(s).
[ View ]

Fixing autocomplete mistake.

Status:Needs review» Needs work

The last submitted patch, drupal8.path-module.1987802-35.patch, failed testing.

Assigned:laurentchardin» Unassigned
  1. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,160 @@
    +use Drupal\Core\Database\Query\PagerSelectExtender;
    +use Drupal\Core\Database\Query\TableSortExtender;
    ...
    +      ->extend('PagerSelectExtender')
    +      ->extend('TableSortExtender');

    This doesn't work like this. It is a string, the full namespaced class must be passed to extend. It's the same as the difference between instanceof and is_subclass_of

  2. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,210 @@
    +   * Constructs a \Drupal\Core\Form\FormBase object.
    +   *
    +   */
    +  public function __construct(Path $path, ModuleHandlerInterface $module_handler, AliasManagerInterface $alias_manager, UrlGeneratorInterface $url_generator) {

    Missing @param

  3. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,210 @@
    +   * @param int $pid
    +   *   an alias id for editing or NULL to add a new one.

    @param int|null $pid
      An alias ID for editing or NULL to add a new one.
  4. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,210 @@
    +
    +    $pathAlias = $this->path->load(array('pid' => $pid));

    Weird blank line, but also $pathAlias needs to be $path_alias

  5. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,210 @@
    +    drupal_set_message(t('The alias has been saved.'));

    $this->t()

  6. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,210 @@
    +    $form_state['redirect'] = 'admin/config/search/path';
    +  }
    +  /**
    +   * {@inheritdoc}

    Missing blank line

  7. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,210 @@
    +    if ($this->getRequest()->query->has('destination')) {
    +      $destination = array('destination' => $this->getRequest()->query->get('destination'));
    +      $this->getRequest()->query->remove('destination');
    +    }

    Let's do $request = $this->getRequest(); first

  8. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,81 @@
    +class PathAdminFilterForm extends FormBase {
    +
    +
    +  /**
    +   * Form constructor.

    Double blank line

  9. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,81 @@
    +   * Form constructor.
    +   *
    +   * @param array $form
    +   *   An associative array containing the structure of the form.
    +   * @param array $form_state
    +   *   An associative array containing the current state of the form.
    +   * @param string $alias
    +   *   A string containing a keyword for searching aliases.
    +   *
    +   * @return array
    +   *   The form structure.
    +   */
    +  public function buildForm(array $form, array &$form_state, $alias = NULL) {
    +    $form['#attributes'] = array('class' => array('search-form'));

    {@inheritdoc}

  10. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,81 @@
    +    $form['basic'] = array('#type' => 'details',
    +      '#title' => $this->t('Filter aliases'),
    +      '#attributes' => array('class' => array('container-inline')),
    +    );

    Let's fix this wrapping

  11. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,81 @@
    +      '#type' => 'submit',
    +      '#value' => $this->t('Filter'),
    ...
    +        '#type' => 'submit',
    +        '#value' => $this->t('Reset'),
    ...
    +    $op = $form_state['values']['op'];
    +    switch ($op) {
    +      case $this->t('Filter'):
    +         $_SESSION['path_admin_filter_form'] = trim($form_state['values']['filter']);
    +        break;
    +      case $this->t('Reset'):
    +        $_SESSION['path_admin_filter_form'] = NULL;
    +        break;
    +    }

    Please just specify two #submit methods:

    '#submit' => array(array($this, 'submitFilter'))
    '#submit' => array(array($this, 'submitReset'))

    And then remove the $op checking and switch/case by having two methods.

  12. +++ b/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php
    @@ -7,6 +7,9 @@
    namespace Drupal\path\Tests;
    +use Drupal\Core\Database\Database;
    +use Drupal\Core\Path\AliasWhitelist;
    +
    /**
      * Tests path alias functionality.

    ?

Status:Needs work» Needs review
StatusFileSize
new8.33 KB
new32.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,473 pass(es), 37 fail(s), and 10 exception(s).
[ View ]

attached patch addresses comments in #37.

Status:Needs review» Needs work

The last submitted patch, drupal8.path-module.1987802-38.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.38 KB
new32.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,353 pass(es).
[ View ]

Fixed a bunch of stuff. @disasm, you have a very bad habit of mismatching your case on object properties! :)

Status:Needs review» Needs work
  1. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -312,4 +312,28 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
    +      ->where('pid <> :pid AND alias = :alias AND langcode = :langcode', array(
    +        ':pid' => $pid,
    +        ':alias' => $alias,
    +        ':langcode' => $langcode))
    +      ->range(0,1);

    Any reason we don't use a db_and?

  2. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,159 @@
    +      ->where('langcode <> :langcode', array(':langcode' => Language::LANGCODE_NOT_SPECIFIED))

    where => condition

  3. +++ b/core/modules/path/path.module
    @@ -58,32 +58,26 @@ function path_menu() {
       $items['admin/config/search/path/add'] = array(
    -    'title' => 'Add alias',
    ...
    -    'access arguments' => array('administer url aliases'),
    -    'type' => MENU_LOCAL_ACTION,
    -    'file' => 'path.admin.inc',
    +    'route_name' => 'path_add',
    +    'type' => MENU_SIBLING_LOCAL_TASK,
    +    'weight' => 1,
       );

    Do we really still need this?

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
new32.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,043 pass(es).
[ View ]

@disasm let me know about this issue.

New patch addresses the condition() and db_and() suggestions.

About the hook_menu() changes, I'm not sure about the desired structure. I tried to remove the whole reference to this path and the title page was removed. Removing only #type and #weight, title worked, but it displayed the help message defined for admin/config/search/path instead of admin/config/search/path/add.

Any suggestion?

Thanks in advance

#43: path-1987802-43.patch queued for re-testing.

Status:Needs review» Needs work
  1. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,217 @@
    +  /**
    +   * The URL generator.
    +   *
    +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
    +   */
    +  protected $urlGenerator;
    ...
    +   * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator
    +   *   The URL generator service.
    ...
    +    $this->urlGenerator = $url_generator;
    ...
    +      $container->get('url_generator')
    ...
    +      '#field_prefix' => $this->urlGenerator->generateFromPath(NULL, array('absolute' => TRUE)),
    ...
    +      '#field_prefix' => $this->urlGenerator->generateFromPath(NULL, array('absolute' => TRUE)),

    Now we can use $this-> urlGenerator() and remove this.

  2. +++ b/core/modules/path/path.module
    @@ -58,32 +58,26 @@ function path_menu() {
    +  // @todo add a paramconverter for path ?

    Please add an issue and reference it here.

Status:Needs work» Needs review
StatusFileSize
new3.21 KB
new31.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,087 pass(es).
[ View ]

Fixed urlGenerator(). Removed the todo, because it really makes no sense. Currently, The pid is passed to a load function on a CRUD service that is returning an associative array with some properties, like alias, language, etc... This cannot be upcasted in it's current state because there is no class to Upcast it to. Ideally, we should completely remove the Path CRUD service, create a model for a PathAlias with the attributes required, and then this model could be upcasted based on the pid, but this is so out of scope for this issue, it's ridiculous.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion, -MENU_LOCAL_ACTION, -FormBase

The last submitted patch, drupal8.path-module.1987802-46.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion, +MENU_LOCAL_ACTION, +FormBase

#46: drupal8.path-module.1987802-46.patch queued for re-testing.

Minor issues.

  1. +++ w/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,159 @@
    +use Drupal\Core\Database\Query\PagerSelectExtender;
    +++ w/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,206 @@
    +use Drupal\Core\Routing\UrlGeneratorInterface;

    Not needed anymore.

  2. +++ w/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -0,0 +1,159 @@
    +      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
    +      ->extend('Drupal\Core\Database\Query\TableSortExtender');

    \Drupal

StatusFileSize
new3.03 KB
new31.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.path-module.1987802-50.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Leaving #2 alone for now. Did a grep of core, and every instance of ->extend does not have a leading \ before Drupal. #1 is addressed. Also, not in the interdiff because it was part of reroll, but routes are now path.

Status:Needs review» Needs work
Issue tags:+Needs reroll, +FormInterface, +WSCCI-conversion, +MENU_LOCAL_ACTION, +FormBase

The last submitted patch, drupal8.path-module.1987802-50.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.17 KB
FAILED: [[SimpleTest]]: [MySQL] 57,073 pass(es), 105 fail(s), and 86 exception(s).
[ View ]

Rerolled. The routes were already created in #2089635: Convert non-test non-form page callbacks to routes and controllers with different identifiers. I retained the identifiers from the previous patch. I also renamed path.delete to path.admin_delete to be consistent with the others.

<<<<<<< HEAD
    'route_name' => 'path.admin_overview',
=======
    'route_name' => 'path.overview',
>>>>>>> 1987802-50

Status:Needs review» Needs work

The last submitted patch, drupal8.path-module.1987802-54.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.09 KB
new32.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,649 pass(es).
[ View ]

Worked some more on this.

  • Tests failed because the route names changed in the merge and I didn't update them in the rest of the code.
  • The title of the "Add Alias" form was not showing up.
  • Converted local tasks to yaml.
  • Removed the entries from hook_menu() that are not needed any more.

The "Add alias" local action is shown on the delete alias confirmation form. It's not clear to me how I can prevent it from showing up there.

Status:Needs review» Needs work
Issue tags:-Needs reroll
  1. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -312,4 +312,28 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
    +    $query = $this->connection->select('url_alias', 'u')

    We can make this db_query.

  2. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,153 @@
    +   * Constructs a PathController object.

    Needs @params

  3. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,153 @@
    +      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
    +      ->extend('Drupal\Core\Database\Query\TableSortExtender');

    \Drupal

  4. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,206 @@
    +   * {@inheritdoc}
    +   * @param int $pid
    +   *   an alias id for editing or NULL to add a new one.

    We can't mix these.

  5. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,112 @@
    +   * Filter Form submission handler.

    Filters form submission handler.

  6. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,112 @@
    +   * Reset Form submission handler.

    Resets form submission handler.

Status:Needs work» Needs review
Issue tags:+Needs reroll
StatusFileSize
new730 bytes
  1. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,153 @@
    +  /**
    +   * Constructs a PathController object.
    +   */
    +  public function __construct(Connection $connection, AliasManagerInterface $alias_manager ) {

    Let's adds some lines of documentation on here.

  2. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,206 @@
    +   * Constructs a \Drupal\Core\Form\FormBase object.

    Lazy copy and paste of existing documentation ...

  3. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,206 @@
    +   * {{@inheritdoc}}

    No double braces needed.

  4. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,206 @@
    +    if ($this->getRequest()->query->has('destination')) {
    +      $destination = array('destination' => $this->getRequest()->query->get('destination'));
    +      $this->getRequest()->query->remove('destination');

    I don't care much but we could store $this->getRequest()->query as local variable, so the code might gets a bit easier to read.

  5. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -0,0 +1,112 @@
    +  /**
    +   * Form constructor.
    +   *
    +   * @param array $form
    +   *   An associative array containing the structure of the form.
    +   * @param array $form_state
    +   *   An associative array containing the current state of the form.
    +   * @param string $alias
    +   *   A string containing a keyword for searching aliases.
    +   *
    +   * @return array
    +   *   The form structure.
    +   */
    +  public function buildForm(array $form, array &$form_state, $alias = NULL) {

    I don't know how to document a form constructor with additional parameters either.

  6. +++ b/core/modules/path/path.routing.yml
    @@ -1,31 +1,28 @@
    -    _title: 'Add alias'
    ...
    +    _form: '\Drupal\path\Form\EditForm'
    ...
    -    _title: 'Edit alias'
    -    _content: '\Drupal\path\Controller\PathController::adminEdit'

    Oh, what is the reason to remove the _title from here?

Here is an interdiff which solves the problem you described, thank you for mentioning it, this is an actual bug in core.

Status:Needs review» Needs work
Issue tags:-Needs reroll

x-post.

Assigned:Unassigned» pfrenssen

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.99 KB
new32.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

From #57:

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -312,4 +312,28 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
+    $query = $this->connection->select('url_alias', 'u')

We can make this db_query.

Do you mean changing it to a $this->connection->query()? What would be the benefit?

From #58:

+++ b/core/modules/path/path.routing.yml
@@ -1,31 +1,28 @@
-    _title: 'Add alias'
...
+    _form: '\Drupal\path\Form\EditForm'
...
-    _title: 'Edit alias'
-    _content: '\Drupal\path\Controller\PathController::adminEdit'
Oh, what is the reason to remove the _title from here?

_title does not work with _form. These routes were initially converted to _content placeholders in #2089635: Convert non-test non-form page callbacks to routes and controllers which do support _title. The titles are being output in $form['#title']. I understand the reasoning behind this, as a form is not strictly linked to a route but can also be placed arbitrarily on a page, and needs to be able to provide a title regardless of the route used.

StatusFileSize
new730 bytes
new33.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,570 pass(es).
[ View ]

Woops forgot to add dawehner's patch that fixes the stray local action. Here it is.

Status:Needs review» Needs work

Do you mean changing it to a $this->connection->query()? What would be the benefit?

It is bit faster then $this->connection->select(). select is normally used for alterable queries.

  1. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -76,8 +76,6 @@ public function getFormID() {
        * {@inheritdoc}

    Parent method has no pid param so we have to document all the params here with function description.

  2. +++ b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php
    @@ -22,17 +22,7 @@ public function getFormID() {
    -   * Form constructor.
    -   *
    -   * @param array $form
    -   *   An associative array containing the structure of the form.
    -   * @param array $form_state
    -   *   An associative array containing the current state of the form.
    -   * @param string $alias
    -   *   A string containing a keyword for searching aliases.
    -   *
    -   * @return array
    -   *   The form structure.
    +   * {@inheritdoc}

    This change is not required.

Assigned:Unassigned» pfrenssen

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.69 KB
new33.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]

Adressed remarks.

Status:Needs review» Reviewed & tested by the community

While manually testing I realized that the source column also contains the path alias but this is not the problem of this issue: #2096135: PathProcessorAlias ignore 'alias' => TRUE

Awesome @pfrenssen thanks for fixing the issue. +1 for RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new608 bytes
new33.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,223 pass(es).
[ View ]

The menu.inc hunk went in elsewhere, the other conflict was translation module being removed.

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new33.31 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, drupal_1987802_72.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new33.16 KB
PASSED: [[SimpleTest]]: [MySQL] 59,265 pass(es).
[ View ]

Another re-roll from #69

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1987802-path-controllers-74.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion, +MENU_LOCAL_ACTION, +FormBase

#74: 1987802-path-controllers-74.patch queued for re-testing.

  1. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -312,4 +312,23 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
    +   *   TRUE if alias exists, false otherwise.

    Nitpick: Should be FALSE here.

  2. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,158 @@
    +   * The connection service.

    I don't think we should call that connection service. This is the database connection.

  3. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,215 @@
    +    $source = &$form_state['values']['source'];

    We mostly use " =& " as code style here.

  4. +++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
    @@ -0,0 +1,215 @@
    +  }
    +}
    diff --git a/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php b/core/modules/path/lib/Drupal/path/Form/PathAdminFilterForm.php

    Let's not forget the empty line here.

"We mostly use " =& " as code style here."

No, quite sure that "= &$..." is correct, to better separate it from &=. Search for e.g. "= &drupal_static" vs. the other way round.

Looks like a lot of the instances that use the other way in core are part of views code...

Oh right, I just thought that > 60 instances of " =& " would be a lot. But I disagree with sticking the "&" to the right expression
in order to show the reference.

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs tests

Both path_admin_filter_form_submit_filter() and path_admin_filter_form_submit_reset() are broken in HEAD, and this papers over that we have no test coverage. Either we open an issue to fix that and block this one, or just do it here.

Status:Needs work» Needs review
StatusFileSize
new3.76 KB
new32.83 KB
FAILED: [[SimpleTest]]: [MySQL] 59,833 pass(es), 2 fail(s), and 8 exception(s).
[ View ]

Re-rolling with fix for #79. Still needs test, just setting review for testbot...

Status:Needs review» Needs work

The last submitted patch, 83: 1987802-path-controllers-83.patch, failed testing.

The last submitted patch, 83: 1987802-path-controllers-83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
new32.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,701 pass(es).
[ View ]

Fixing fails...

Looks great only minor doc issues.

  1. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,158 @@
    +   * When filter key passed, perform a standard search on the given key,
    +   * and return the list of matching URL aliases.

    I think we can expand it to 80 chars.

  2. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,158 @@
    +    // @todo Implement the session service: http://drupal.org/node/1858196

    Full stop missing :). I think Implement should be with small i.

  3. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,158 @@
    +  protected $databaseConnection;
    ...
    +   * @param \Drupal\Core\Database\Connection $database_connection
    ...
    +  public function __construct(Connection $database_connection, AliasManagerInterface $alias_manager ) {
    +    $this->databaseConnection = $database_connection;

    Why not just database?

  4. +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,158 @@
    +   * The connection service.

    The database connection service.

  5. +++ /dev/null
    @@ -1,327 +0,0 @@
    - * Returns a listing of all defined URL aliases.
    +++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
    @@ -7,34 +7,158 @@
    +   * The admin overview.

    I think we should restore the original desc

Assigned:Unassigned» pfrenssen
Status:Needs review» Needs work
StatusFileSize
new32.86 KB

Going to address the above remarks. Here is already a straight reroll to latest HEAD.

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new32.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1987802-path-controllers-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new6.93 KB

Addressed the remarks from #87. Also went over the patch with a fine comb and straightened out some last minor kinks.

+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
@@ -7,34 +7,158 @@
+ // @todo Implement the session service: http://drupal.org/node/1858196
Full stop missing :). I think Implement should be with small i.

The first word following a @todo ("Implement") should be capitalized according to coding standards, but the link should be on a separate @see line. Fixed it.

The last submitted patch, 89: 1987802-path-controllers-89.patch, failed testing.

The last submitted patch, 89: 1987802-path-controllers-89.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 89: 1987802-path-controllers-89.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,900 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
new2.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,862 pass(es).
[ View ]

Rerolled on current HEAD. Main changes:

  • uses Path service instead of querying the DB
  • no longer tries to removes hook_menu, already removed,
  • use Drupal::languageManager.

The last submitted patch, 94: 1987802-path-controllers-94.patch, failed testing.

StatusFileSize
new32.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,870 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new266 bytes

This should remove most errors, though probably not all.

Status:Needs review» Needs work

The last submitted patch, 96: 1987802-96-path-admin.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,856 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new1.86 KB

Path::__construct() must type hint on PathInterface now that it exists, not Path. This is needed for the MongoDB service override.

Status:Needs review» Needs work

The last submitted patch, 98: 1987802-98.patch, failed testing.

Assigned:Unassigned» tim.plunkett

Will work on this and the tests.