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.

CommentFileSizeAuthor
#104 interdiff.txt620 bytestim.plunkett
#104 path-1987802-104.patch27.89 KBtim.plunkett
#102 path-1987802-102.patch27.93 KBtim.plunkett
#98 interdiff-1987802-94-98.txt1.86 KBfgm
#98 1987802-98.patch32.45 KBfgm
#96 interdiff.txt266 bytesfgm
#96 1987802-96-path-admin.patch32.42 KBfgm
#94 interdiff-89-94.patch2.33 KBfgm
#94 1987802-path-controllers-94.patch32.39 KBfgm
#89 interdiff.txt6.93 KBpfrenssen
#89 1987802-path-controllers-89.patch32.88 KBpfrenssen
#88 1987802-path-controllers-88.patch32.86 KBpfrenssen
#86 1987802-path-controllers-86.patch32.86 KBvijaycs85
#86 1987802-diff-83-86.txt1.09 KBvijaycs85
#83 1987802-path-controllers-83.patch32.83 KBvijaycs85
#83 1987802-diff-74-83.txt3.76 KBvijaycs85
#74 1987802-path-controllers-74.patch33.16 KBvijaycs85
#72 drupal_1987802_72.patch33.31 KBXano
#69 path-1987802-69.patch33.16 KBtim.plunkett
#69 interdiff.txt608 bytestim.plunkett
#65 drupal8.path-module.1987802-65.patch33.85 KBpfrenssen
#65 interdiff.txt2.69 KBpfrenssen
#62 drupal8.path-module.1987802-62.patch33.24 KBpfrenssen
#62 interdiff.txt730 bytespfrenssen
#61 drupal8.path-module.1987802-61.patch32.52 KBpfrenssen
#61 interdiff.txt4.99 KBpfrenssen
#58 interdiff.txt730 bytesdawehner
#56 drupal8.path-module.1987802-56.patch32.76 KBpfrenssen
#56 interdiff.txt4.09 KBpfrenssen
#54 drupal8.path-module.1987802-54.patch32.17 KBpfrenssen
#50 drupal8.path-module.1987802-50.patch31.83 KBdisasm
#50 interdiff.txt3.03 KBdisasm
#46 drupal8.path-module.1987802-46.patch31.82 KBdisasm
#46 interdiff.txt3.21 KBdisasm
#43 path-1987802-43.patch32.21 KBplopesc
#43 interdiff.txt1.67 KBplopesc
#40 path-1987802-40.patch32.25 KBtim.plunkett
#40 interdiff.txt4.38 KBtim.plunkett
#38 drupal8.path-module.1987802-38.patch32.05 KBdisasm
#38 interdiff.txt8.33 KBdisasm
#35 drupal8.path-module.1987802-35.patch31.51 KBdisasm
#35 interdiff.txt1.4 KBdisasm
#34 drupal8.path-module.1987802-34.patch31.53 KBdisasm
#34 interdiff.txt3.29 KBdisasm
#32 drupal8.path-module.1987802-32.patch31.16 KBdisasm
#32 interdiff.txt1.06 KBdisasm
#31 drupal8.path-module.1987802-31.patch31.14 KBdisasm
#31 interdiff.txt6.71 KBdisasm
#29 drupal8.path-module.1987802-29.patch30.6 KBdisasm
#29 interdiff.txt6.24 KBdisasm
#27 drupal8.path-module.1987802-27.patch30.75 KBdisasm
#27 interdiff.txt5.29 KBdisasm
#24 drupal8.path-module.1987802-24.patch30.08 KBdisasm
#24 interdiff.txt13.03 KBdisasm
#20 drupal-1987802-20.patch31.17 KBlaurentchardin
#15 drupal-1987802-15.patch16.13 KBlaurentchardin
#12 drupal-1987802-12.patch16.19 KBlaurentchardin
#9 drupal-1987802-9.patch17.15 KBlaurentchardin
#8 drupal-1987802-8.patch15.34 KBlaurentchardin
#5 drupal-1987802-5.patch8.64 KBlaurentchardin
#2 drupal-convert_path_admin_overview-1987802-2.patch8.61 KBh3rj4n
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

h3rj4n’s picture

Assigned: Unassigned » h3rj4n
h3rj4n’s picture

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

vijaycs85’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

laurentchardin’s picture

Status: Needs work » Needs review
FileSize
8.64 KB

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

Status: Needs review » Needs work

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

dawehner’s picture

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.

laurentchardin’s picture

Status: Needs work » Needs review
FileSize
15.34 KB

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.
laurentchardin’s picture

FileSize
17.15 KB

Rerolling patch.

dawehner’s picture

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

laurentchardin’s picture

FileSize
16.19 KB

Rerolling patch.

laurentchardin’s picture

Status: Needs work » Needs review

Forgot to change the status.

dawehner’s picture

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.

laurentchardin’s picture

FileSize
16.13 KB

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.

dawehner’s picture

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

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

There we go

laurentchardin’s picture

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.

laurentchardin’s picture

Assigned: Unassigned » laurentchardin
Status: Needs work » Needs review
Issue tags: +MENU_LOCAL_ACTION
FileSize
31.17 KB

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 ** :

laurentchardin’s picture

dawehner’s picture

Regarding the service: use the cached version of it.

Crell’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
13.03 KB
30.08 KB

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.

tim.plunkett’s picture

  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

disasm’s picture

Status: Needs work » Needs review
FileSize
5.29 KB
30.75 KB

Addressing most of #26.

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review
FileSize
6.24 KB
30.6 KB

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.

jibran’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
31.14 KB

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.

disasm’s picture

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

jibran’s picture

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

disasm’s picture

Fixing typo, adding urlGenerator to EditForm.

disasm’s picture

Fixing autocomplete mistake.

Status: Needs review » Needs work

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

tim.plunkett’s picture

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.
    

    ?

disasm’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
32.05 KB

attached patch addresses comments in #37.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
32.25 KB

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

dawehner’s picture

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?

xjm’s picture

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.

plopesc’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
32.21 KB

@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

jibran’s picture

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

jibran’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
31.82 KB

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.

disasm’s picture

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

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

disasm’s picture

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.

jibran’s picture

Berdir’s picture

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.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
32.17 KB

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.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
32.76 KB

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.

jibran’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll
FileSize
730 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.

jibran’s picture

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

x-post.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
4.99 KB
32.52 KB

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.

pfrenssen’s picture

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

jibran’s picture

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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
2.69 KB
33.85 KB

Adressed remarks.

dawehner’s picture

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

jibran’s picture

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

alexpott’s picture

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

Patch no longer applies.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
608 bytes
33.16 KB

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

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

Patch no longer applies.

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
33.31 KB

Re-roll.

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
33.16 KB

Another re-roll from #69

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Xano’s picture

Status: Reviewed & tested by the community » Needs work

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

Xano’s picture

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

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

dawehner’s picture

  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.

Berdir’s picture

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

dawehner’s picture

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.

tim.plunkett’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
32.83 KB

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
32.86 KB

Fixing fails...

jibran’s picture

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

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Status: Needs review » Needs work
FileSize
32.86 KB

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

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
32.88 KB
6.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.

andypost’s picture

Status: Needs review » Needs work

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

fgm’s picture

Status: Needs work » Needs review
FileSize
32.39 KB
2.33 KB

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.

fgm’s picture

FileSize
32.42 KB
266 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.

fgm’s picture

Status: Needs work » Needs review
FileSize
32.45 KB
1.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.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Will work on this and the tests.

tim.plunkett’s picture

Status: Needs work » Postponed
Issue tags: -Needs tests
tim.plunkett’s picture

Status: Postponed » Needs review
Issue tags: -MENU_LOCAL_ACTION
FileSize
27.93 KB

Opened #2244861: Four public methods on \Drupal\Core\Path\AliasStorage aren't on the interface. Not a blocker, but its stranger now that we're typehinting.

Status: Needs review » Needs work

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

tim.plunkett’s picture

FileSize
27.89 KB
620 bytes

HAH.

tim.plunkett’s picture

Status: Needs work » Needs review

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Much more cleaner.

+++ b/core/modules/path/lib/Drupal/path/Form/EditForm.php
@@ -0,0 +1,65 @@
+  public function deleteSubmit(array &$form, array &$form_state) {
+    $form_state['redirect_route'] = new Url('path.delete', array(
+      'pid' => $form_state['values']['pid'],
+    ));
+
+    if ($this->getRequest()->query->has('destination')) {
+      $form_state['redirect_route']->setOption('query', drupal_get_destination());
+      $this->getRequest()->query->remove('destination');
+    }

minor, seems better to use else

kim.pepper’s picture

104: path-1987802-104.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 93e7749 and pushed to 8.x. Thanks!

  • Commit 93e7749 on 8.x by alexpott:
    Issue #1987802 by disasm, pfrenssen, laurentchardin, tim.plunkett, fgm,...
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Thanks!

Status: Fixed » Closed (fixed)

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