Problem/Motivation

Convert all instances of drupal_get_destination() to use the new service, see https://www.drupal.org/node/2448603

Proposed resolution

There are three type of changes made.

1) For the procedural code in .module files - and for classes with static functions

- 'query' => drupal_get_destination(),
+ 'query' => \Drupal::destination()->getAsArray(),

2) For Services registered in the Container and classes that extends EntityListBuilder the redirect.destination has been injected.

3) For application level code a new trait has been employed. For example the FormBase and the ControllerBase make use of it.

With an eye for future expansion ControllerBase makes use of the trait - even though current only two classes that directly extend it make use of this facility.

Removing this global function call - has the benefit that it is now possible to unit test with conventional mocks, therefore it reduces fragility.
Previously tests polluted the global namespace with test functions - which is less than ideal.

Remaining tasks

None

User interface changes

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because Drupal will work without making this change.
Issue priority Normal because this removes usages of a deprecated function but there will remain the backwards-compatible function.
Disruption Not disruptive for core/contributed and custom modules/themes because the BC function will remain through 8.x.
BUT this patch touches 49 files so some disruption to the issue queue is expected
CommentFileSizeAuthor
#108 interdiff.txt1.83 KBklausi
#107 2448605.patch48.93 KBklausi
#103 replace-2448605-103.patch49.56 KBwillzyx
#99 replace-2448605-99.patch49.54 KBmartin107
#95 replace-2448605-95.patch50.22 KBmartin107
#91 replace-2448605-91.patch50.17 KBtim.plunkett
#91 interdiff.txt799 bytestim.plunkett
#85 replace-2448605-85.patch50.17 KBmartin107
#85 interdiff-84-85.txt648 bytesmartin107
#84 replace-2448605-84.patch50.19 KBwillzyx
#84 interdiff-76-84.txt2.18 KBwillzyx
#76 replace-2448605-76.patch50.24 KBmartin107
#76 interdiff-74-76.txt750 bytesmartin107
#74 interdiff-65-74.txt17.72 KBmartin107
#74 replace-2448605-74.patch49.69 KBmartin107
#66 interdiff-63-65.txt6.05 KBmartin107
#65 intediff-59-63.txt8.63 KBmartin107
#65 replace-2448605-65.patch49.64 KBmartin107
#63 intediff-59-63.txt8.63 KBmartin107
#63 replace-2448605-63.patch50.82 KBmartin107
#59 interdiff-55-59.txt5.18 KBmartin107
#59 replace-2448605-59.patch54.85 KBmartin107
#55 replace-2448605-55.patch55.5 KBmartin107
#55 interdiff-52-55.txt26.78 KBmartin107
#52 interdiff-44-52.txt32.57 KBmartin107
#52 replace-2448605-52.patch67.82 KBmartin107
#51 RedirectDestinationTrait-2448605-51-do-not-test.patch2.42 KBwillzyx
#48 LinkInheritanceDiagramCropped.png2.3 KBmartin107
#48 LinkInheritanceDiagram.png6.91 KBmartin107
#45 interdiff-2448605-42-44.txt1.58 KBwillzyx
#44 replace-2448605-44.patch73.69 KBwillzyx
#44 replace-2448605-44.patch73.69 KBwillzyx
#42 replace-2448605-42.patch71.95 KBwillzyx
#42 interdiff-2448606-40-42.txt465 byteswillzyx
#40 interdiff-38-40.txt2.07 KBmartin107
#40 replace-2448605-40.patch71.73 KBmartin107
#38 interdiff-36-38.txt3.39 KBmartin107
#38 replace-2448605-38.patch69.66 KBmartin107
#36 interdiff-35-36.txt18.2 KBmartin107
#36 replace-2448605-36.patch71.15 KBmartin107
#35 replace-2448605-35.patch62.87 KBmartin107
#35 interdiff-32-35.txt15.24 KBmartin107
#32 interdiff-30-32.txt1.31 KBmartin107
#32 replace-2448605-32.patch54.84 KBmartin107
#30 interdiff-28-30.txt604 bytesmartin107
#30 replace-2448605-30.patch53.54 KBmartin107
#28 replace-2448605-28.patch53.17 KBmartin107
#28 interdiff-26-28.txt2.35 KBmartin107
#26 replace-2448605-26.patch50.82 KBmartin107
#26 interdiff-24-26.txt12.17 KBmartin107
#24 replace-2448605-24.patch42.13 KBmartin107
#24 interdiff-20-24.txt1016 bytesmartin107
#20 interdiff-18-20.txt3.61 KBmartin107
#20 replace-2448605-20.patch41.14 KBmartin107
#18 replace-2448605-18.patch37.53 KBmartin107
#18 interdiff-16-18.txt3.66 KBmartin107
#16 interdiff-12-16.txt11.48 KBmartin107
#16 replace-2448605-16.patch36.14 KBmartin107
#13 replace-2448605-12.patch28.12 KBdpopdan
#11 replace-2448605-11.patch33.98 KBmartin107
#6 replace-2448605-6.patch33.67 KBmartin107
#2 replace-2448605-2.patch27.54 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

Assigned: Unassigned » martin107

Just a comment before I make a start on this

To help define the size of the task and its potential disruption

searching for drupal_get_destination() in drupal

55 matches in 47 files.

DefaultExceptionHtmlSubscriber::drupalGetDestination() is a wrapper for drupal_get_destination()

Not a problem just a comment.

martin107’s picture

Status: Active » Postponed
FileSize
27.54 KB

So in the interests of transparency, other than a global replace, here is what tweaks I have made.

A) I have flattened/removed a function in ForumController
The drupal_get_destinations wrapper was protected and being called only once so the natural thing to do seemed to replace it with out bright and shiney new global wrapper.

B) Same for the two calls to the protected function DefaultExceptionHtmlSubscriber::drupalGetDestination()

No sense in wrapping a function twice.

C) While reading some updates I replaced the function calls with language about the service just to make to comments scan correctly.

I will un-postpone and test when #2426805: Modernize drupal_get_destination() is committed.

tim.plunkett’s picture

Scrolling through quickly, most of those places need to use dependency injection instead of \Drupal::service

martin107’s picture

Ah that is great news,

I was too timid to do that given we are in beta ... I am happy to do this properly.

martin107’s picture

Issue tags: +Kill includes
martin107’s picture

FileSize
33.67 KB

Taking small bite sized chunk out of this issue.

First step - cherry pick all the uncontroversial changes - that is classes with a pre-existing create() function and easy access to a "container"

I am trying to establishing a cookie cutter ... a template to make all these similar changes identical... I will use the changes to BookAdminEditForm as the pattern.

Its just a first iteration of this step a lot more to do and a few steps after

Just posting early and posting often... seems like a good thing to do.

More tomorrow.

tim.plunkett’s picture

Status: Postponed » Needs review

The last submitted patch, 2: replace-2448605-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: replace-2448605-6.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/core/modules/book/src/Form/BookAdminEditForm.php
    @@ -38,16 +38,26 @@ class BookAdminEditForm extends FormBase {
    +    $this->destinaiton = $destination;
    

    Typo

  2. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -167,7 +178,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $type = '
    +    $destination = \Drupal::service('redirect.destination')->getAsArray();
    

    This should use the injected values

There are still more places to inject it. And instead of calling getAsArray() on the service in the constructor, I would inject the whole service and call the method inline.

martin107’s picture

Status: Needs work » Needs review
FileSize
33.98 KB

My time to work on this this evening is suddenly cut short....bugger

I would inject the whole service and call the method inline.

Its was a minor point, I was trying to skirt around issues to do with the law of demeter and The Principle of Least Knowledge.

The one where if you need a class to receive a money class and you pass in a wallet class containing the money class then you doing it wrong.

Given the create function is already part of the class and the reality is that these classes cannot exist outside the Drupal ecosystem. I will go back tomorrow and call things inline as most drupalers would expect.

For now I have just done a reroll and fixed 10.1 and 10.2

Status: Needs review » Needs work

The last submitted patch, 11: replace-2448605-11.patch, failed testing.

dpopdan’s picture

FileSize
28.12 KB
dawehner’s picture

Status: Needs work » Needs review

Let's test the patch.

Status: Needs review » Needs work

The last submitted patch, 13: replace-2448605-12.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
36.14 KB
11.48 KB

Ok so starting from #13... I have converted 5 classes to the new 'inject the service' way of doing things.

more daily...

Status: Needs review » Needs work

The last submitted patch, 16: replace-2448605-16.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
37.53 KB

1) Cleared a typo, getAsArrays

2) Added missing element from create() functions

3) Added @redirect.destination parameter to the exception.default_html service definition....

Just fixing my errors as I see them. It does not solve the main recurring error message, but changes the code enough that I want to see what testbot has to say

BTW there is an easy way to get a stack trace with testing, just visit a non-existent page such as 'node/xyx'

I will look at this some more tonight.... in about 5 hours time.

Status: Needs review » Needs work

The last submitted patch, 18: replace-2448605-18.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
41.14 KB
3.61 KB

Refactoring CustomPageExceptionHtmlSubscriberTest a little :)

DefaultExceptionHtmlSubscriber::drupalGetDestination() was a wrapper function which has been removed.

This obviates the need for TestCustomPageExceptionHtmlSubscriber which was just a classes used to overload the wrapper.

In short the test wrapper has been turned into the equivalent mock function. No biggie.

Still not getting to the heart of the recurring fails..

but CustomPageExceptionHtmlSubscriberTest now passes.

Status: Needs review » Needs work

The last submitted patch, 20: replace-2448605-20.patch, failed testing.

willzyx’s picture

We can use \Drupal::destination() instead of \Drupal::service('redirect.destination')

martin107’s picture

Ok thanks, I will make the changes as I go.

I was planning on removing as many of those types of calls as possible, and using dependency injection.

They should only really be a handful of those sticking plasters in the end in .inc files or where a API would have to change.

martin107’s picture

Status: Needs work » Needs review
FileSize
1016 bytes
42.13 KB

This should bring the error count down :)

Status: Needs review » Needs work

The last submitted patch, 24: replace-2448605-24.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
12.17 KB
50.82 KB

Converted another 5 classes.

Status: Needs review » Needs work

The last submitted patch, 26: replace-2448605-26.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
53.17 KB

1) fixed my mistake, must provide mocks for the extra parameter I added

core/modules/node/tests/src/Unit/Plugin/views/field/NodeBulkFormTest.php
core/modules/user/tests/src/Unit/Plugin/views/field/UserBulkFormTest.php

These tests now pass.

2) Looking at other issues tagged "Kill Includes" I see they are also tagged beta target ...
Can someone comment if that is appropriate here too.

3) I haven't settled this thought yet but I think I maybe suffering from random test failures.

Dont get me wrong most of the failures above are my fault by the one with the text

Argument 1 passed to Drupal\Core\Theme\ThemeManager::setActiveTheme() must be an instance of Drupal\Core\Theme\ActiveTheme, null given, called

well I am getting identical errors popping up in another unrelated issue.

https://www.drupal.org/node/2448915#comment-9709727

Status: Needs review » Needs work

The last submitted patch, 28: replace-2448605-28.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
53.54 KB
604 bytes

UserAdminListingTest now passes.

Status: Needs review » Needs work

The last submitted patch, 30: replace-2448605-30.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
54.84 KB
1.31 KB

ForumTest now passes.

Status: Needs review » Needs work

The last submitted patch, 32: replace-2448605-32.patch, failed testing.

martin107’s picture

Looking at @RednderElement and @ViewsFIeld plugins affected by this change - they cannot be converted using the pattern I have been using up to now.

On the @ViewsField side we have

LinkDelete, RevisionLinkDelete, UserLoginBlock, LinkCancel, LinkEdit, EntityOperation, Links, BulkForm

Which all need a redirect.destination service

Just wondering out loud if that list demonstrates the need for the service to added by default to the plugin base some how?

In any event this is guaranteed to break the API - and most definitely a separate 8.1 task

I will open up an issue if people think this is the correct way forward?

Just making a list of remaining tasks ( which I am actively working on ) :-

ForumController and MenuForm - need converting as normal.
*.module files need conversion to the \Drupal::destination() way of doing things.
Last error needs resolving.

martin107’s picture

Status: Needs work » Needs review
FileSize
15.24 KB
62.87 KB

Ok so my modification of EntityOperations.php is a template for how to modify @Views field plugins .. it is not complicated.

I have converted 3 classes, fixed the error and systematically modified any *.module files into their final form.

More tomorrow.

Patch size is slowly creeping up with many more classes to come. :)

martin107’s picture

In this iteration, I have taken all the plugins related to links such as RevisionsLinksRevert, LinkApprove and converted them.

[ There is lots to potentially go wrong so I am expecting some failures... ]

Status: Needs review » Needs work

The last submitted patch, 36: replace-2448605-36.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
69.66 KB
3.39 KB

1) I am a little stumped, so this is a request for help.

\Drupal::destination()->getAsArray();

It is ok to leave these in .module files - BUT at the moment I can't get rid of the one in

/core/modules/views/src/Plugin/views/field/Links.php

constructive advice would be helpful. Maybe a intermediate baseClass?

2) Removed a stray new line, I was semi-systematically inserting.

1 remaining test will fail.

Status: Needs review » Needs work

The last submitted patch, 38: replace-2448605-38.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
Issue tags: +beta target
FileSize
71.73 KB
2.07 KB

I have opened up a issue to replace usages of drupal_get_destination in devel in the same way #2452047: drupal_get_destination is deprecated
If anyone else has a favourite module that they would like converted ... just let me know :)

Minor changes only

1) Converting mentions to drupal_get_destination from comments

2) ViewAjaxControllerTest removed redundant code.

Expect a single failure.

Status: Needs review » Needs work

The last submitted patch, 40: replace-2448605-40.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
465 bytes
71.95 KB

this should fix the fail

Status: Needs review » Needs work

The last submitted patch, 42: replace-2448605-42.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
73.69 KB
73.69 KB

ehm..for real now

willzyx’s picture

FileSize
1.58 KB

I had forgotten to attach the interdiff

martin107’s picture

Just pointing out a typo I introduced in #40 I could not spell redirect.

+++ b/core/modules/comment/src/CommentManager.php
@@ -159,7 +159,7 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
}

if ($this->authenticatedCanPostComments) {
- // We cannot use the direct.destination service here because these links
+ // We cannot use the redirect.destination service here because these links
// sometimes appear on /node and taxonomy listing pages.
if ($entity->get($field_name)->getFieldDefinition()->getSetting('form_location') == CommentItemInterface::FORM_SEPARATE_PAGE) {

I will fix this on the next iteration, just wanted to signal my mistake early.

willzyx’s picture

I'm wondering whether it would be better to create RedirectDestinationTrait, instead of continuing on this path

from #2134513: [policy, no patch] Trait strategy

Remember, traits are *not* an alternative for proper object composition. Their main use is for reducing boilerplate code and providing utility code without relying on inheritance.

and I think this might be the case

martin107’s picture

thanks willzyx for the fix...

That certainly does the job. but I would prefer a different way forward.

I am uploading a inheritance diagram and a cropped version showing the detail i want to expose for Drupal\user\Plugin\views\field\Link

I think we should place all the construct / create gubbins in the Link class and then let the three classes that extend Link obtaining it up in a more maintainable way.

In short I want to remove the changes to ContactLink and fix the test.

I am running out of time today ... I will try and post an alternative before Sunday night.

martin107’s picture

Sorry willzyx I postred #48 before seeing #47 ... a trait sound like a good idea
as fundamentally there are going to be many link plugins

and many will need the concept of redirect.... more tomorrow.

martin107’s picture

Looking for a way to use a trait

1) There are 3 instances where FieldPluginBase is extended to use a constructor of the form

+ public function __construct(array $configuration, $plugin_id, $plugin_definition, RedirectDestinationInterface $redirect_destination) {

2) There are 2 instances where we could have a trait to cover constructors of the form

+ public function __construct(ModuleHandlerInterface $module_handler, EntityManagerInterface $entity_manager, RedirectDestinationInterface $redirect_destination) {

Every other constructor change appears to be a unique snowflake.... in short I am not sure that a Trait is the way to go.....

willzyx’s picture

I'm not sure.. If we create RedirectDestinationTrait and insert it in Drupal\Core\Form\FormBase and in Drupal\user\Plugin\views\field\Link we can (almost) avoid changing constructors. It would simplify and make the code more clean imho
Attached a draft of RedirectDestinationTrait

martin107’s picture

@willzyx yes, that is a much cleaner change

I especially like the class comment at the top of RedirectDestinationTrait about application-level code..

[ I HAD a concern that we now have a hidden dependency on a call to \Drupal::destination() rather than explicit dependency injection which I prefer BUT the advantages wash all that away. ]

This is not a complete change ... I just want to break it down into manageable chunks...in between testing.

I have rolled #51 with #44 and then refactored all the plugins to use traits

I will get round to all the Form changes later ... after I mop up any failures.

willzyx’s picture

yes.. use RedirectDestinationTrait simplifies a lot this task

+++ b/core/modules/comment/src/Plugin/views/field/LinkApprove.php
@@ -21,6 +22,8 @@
 class LinkApprove extends Link {
 
+    use RedirectDestinationTrait;
+

+++ b/core/modules/comment/src/Plugin/views/field/LinkDelete.php
@@ -19,6 +20,8 @@
 class LinkDelete extends Link {
 
+  use RedirectDestinationTrait;
+

+++ b/core/modules/comment/src/Plugin/views/field/LinkEdit.php
@@ -19,6 +20,8 @@
 class LinkEdit extends Link {
 
+  use RedirectDestinationTrait;
+

Why don't move RedirectDestinationTrait directly in \Drupal\user\Plugin\views\field\Link?

Status: Needs review » Needs work

The last submitted patch, 52: replace-2448605-52.patch, failed testing.

martin107’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
26.78 KB
55.5 KB

A) Fixed the failing test

B) Added a trait to FormBase, and refactored where appropriate ( approx 9 classes )
[ Full disclosure: As an out of scope change I put the 5 FormBase traits into alphabetical order. ]

C) Modified all 3 extendable Link base classes to use our new trait.
This has the slightly negative implication that LinkRely.php now inherits methods it does not use.
But overall the Link classes commonly use the trait so I think the overall benefit is positive.

D) Refactored Overview.php

E) Fixed typo I pointed out in #46

F) Update Issue summary to justify three forms of change,
1) procedural changes in *.module file
2) dependancy injection code for controller and core classes.
3) trait for application level code.

Lots of changes ... would be surprised if every test passes.

I will fix up any errors, and have a further-review before unassigning myself. but that in next week.

martin107’s picture

Issue summary: View changes
willzyx’s picture

great work. Some observations:

  1. +++ b/core/modules/book/src/Form/BookAdminEditForm.php
    @@ -23,6 +24,8 @@
    +  use RedirectDestinationTrait;
    +
    

    isn't needed since BookAdminEditForm extends FormBase

  2. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
    @@ -63,7 +63,7 @@ public static function preRenderCompactLink($element) {
    +        'query' => $this->getDestination(),
    
    @@ -71,7 +71,7 @@ public static function preRenderCompactLink($element) {
    +        'query' => $this->getDestination(),
    

    $this is not accessible in static context

  3. +++ b/core/modules/locale/locale.pages.inc
    @@ -121,5 +121,5 @@ function template_preprocess_locale_translation_last_check(array &$variables) {
    +  $variables['link'] = \Drupal::l(t('Check manually'), new Url('locale.check_translation', array(), array('query' => \Drupal::service('redirect.destination')->getAsArray())));
    

    and

    +++ b/core/modules/system/tests/modules/common_test/src/Controller/CommonTestController.php
    @@ -89,7 +89,7 @@ public function jsAndCssQuerystring() {
    +    $destination = \Drupal::service('redirect.destination')->getAsArray();
    

    and

    +++ b/core/modules/user/user.api.php
    @@ -136,7 +136,7 @@ function hook_user_login($account) {
    +    drupal_set_message(t('Configure your <a href="@user-edit">account time zone setting</a>.', array('@user-edit' => $account->url('edit-form', array('query' => \Drupal::service('redirect.destination')->getAsArray(), 'fragment' => 'edit-timezone')))));
    

    Should be \Drupal::destination()->getAsArray();

  4. +++ b/core/lib/Drupal/Core/Routing/RedirectDestinationTrait.php
    @@ -0,0 +1,75 @@
    +  /**
    +   * Sets the redirect destination service.
    +   *
    +   * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination
    +   *   The link generator service.
    +   *
    +   * @return $this
    +   */
    

    my bad.. should be "The redirect destination service."

  5. +++ b/core/modules/node/src/Plugin/views/field/LinkDelete.php
    @@ -19,6 +20,8 @@
     class LinkDelete extends Link {
     
    +  use RedirectDestinationTrait;
    +
    
    +++ b/core/modules/node/src/Plugin/views/field/LinkEdit.php
    @@ -20,6 +21,8 @@
     class LinkEdit extends Link {
     
    +  use RedirectDestinationTrait;
    +
    
    +++ b/core/modules/node/src/Plugin/views/field/RevisionLink.php
    @@ -23,6 +24,8 @@
     class RevisionLink extends Link {
     
    +  use RedirectDestinationTrait;
    +
    

    Why don't move RedirectDestinationTrait directly in \Drupal\user\Plugin\views\field\Link?

Status: Needs review » Needs work

The last submitted patch, 55: replace-2448605-55.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
54.85 KB
5.18 KB

Last changes tonight,

not all tests fixed, and not all suggestions implemented

Just want to reduce the bulk of the errors overnight so I can see the stragglers.

will post a detailed explanation in the morning.

Status: Needs review » Needs work

The last submitted patch, 59: replace-2448605-59.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

This looks pretty great!

  1. +++ b/core/lib/Drupal/Core/Form/FormBase.php
    @@ -22,9 +23,11 @@
    diff --git a/core/lib/Drupal/Core/Render/Element/Link.php b/core/lib/Drupal/Core/Render/Element/Link.php
    
    diff --git a/core/lib/Drupal/Core/Render/Element/Link.php b/core/lib/Drupal/Core/Render/Element/Link.php
    index c6714be..73ad3c42 100644
    
    index c6714be..73ad3c42 100644
    --- a/core/lib/Drupal/Core/Render/Element/Link.php
    
    --- a/core/lib/Drupal/Core/Render/Element/Link.php
    +++ b/core/lib/Drupal/Core/Render/Element/Link.php
    
    +++ b/core/lib/Drupal/Core/Render/Element/Link.php
    +++ b/core/lib/Drupal/Core/Render/Element/Link.php
    @@ -7,6 +7,7 @@
    
    @@ -7,6 +7,7 @@
     
     namespace Drupal\Core\Render\Element;
     
    +use Drupal\Core\Routing\RedirectDestinationTrait;
     use Drupal\Component\Utility\NestedArray;
     use Drupal\Component\Utility\Html as HtmlUtility;
     
    @@ -17,6 +18,8 @@
    
    @@ -17,6 +18,8 @@
      */
     class Link extends RenderElement {
     
    +  use RedirectDestinationTrait;
    +
       /**
        * {@inheritdoc}
        */
    

    What is the reason to add the trait without using it at all?

  2. +++ b/core/modules/forum/src/Controller/ForumController.php
    @@ -67,6 +68,13 @@ class ForumController extends ControllerBase {
    @@ -83,8 +91,10 @@ class ForumController extends ControllerBase {
    
    @@ -83,8 +91,10 @@ class ForumController extends ControllerBase {
        *   Array of active fields on the site.
        * @param \Drupal\Core\Entity\EntityStorageInterface $node_type_storage
        *   Node type storage handler.
    +   * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination
    +   *   The redirect destination service.
        */
    -  public function __construct(ForumManagerInterface $forum_manager, VocabularyStorageInterface $vocabulary_storage, TermStorageInterface $term_storage, AccountInterface $current_user, EntityAccessControlHandlerInterface $node_access, array $field_map, EntityStorageInterface $node_type_storage) {
    +  public function __construct(ForumManagerInterface $forum_manager, VocabularyStorageInterface $vocabulary_storage, TermStorageInterface $term_storage, AccountInterface $current_user, EntityAccessControlHandlerInterface $node_access, array $field_map, EntityStorageInterface $node_type_storage, RedirectDestinationInterface $redirect_destination) {
         $this->forumManager = $forum_manager;
         $this->vocabularyStorage = $vocabulary_storage;
         $this->termStorage = $term_storage;
    @@ -92,6 +102,7 @@ public function __construct(ForumManagerInterface $forum_manager, VocabularyStor
    
    @@ -92,6 +102,7 @@ public function __construct(ForumManagerInterface $forum_manager, VocabularyStor
         $this->nodeAccess = $node_access;
         $this->fieldMap = $field_map;
         $this->nodeTypeStorage = $node_type_storage;
    +    $this->redirectDestination = $redirect_destination;
       }
     
       /**
    

    What about providing this as part of the ControllerBase as well?

martin107’s picture

It is unusual for the review to point out bug fixes ( #57 pointed out the bugs before testbot could report them! ) -- thank you both...

What about providing this as part of the ControllerBase as well?

Certainly a possibility, clearly application level code.... There are also strong parallels between the traits in FormBase and ControllerBase.

I also want to layout the case for NOT making the change.....

FormBase is different from ControllerBase

In FormBase, it is true that redirect is a common almost universal pattern, I cannot make the same case for ControllerBase.

I can think of lots of application level controllers I want to write - for which to concept of redirect is entirely alien...

If added it certainly would not hurt any controllers I can conceive of creating, but keeping to universal good - is a way to reduce clutter.

If we do not include it then the small burden we place of the developers head is that he/she has to know just to add 2

use Drupal\Core\Routing\RedirectDestinationTrait; like statement before $this->getDestination() become available.

Perhaps we could document this explicitly somewhere.

Anyway, I am still forming my opinion, it is a small point in any event ... I would also like to hear other voices.

martin107’s picture

Issue summary: View changes
FileSize
50.82 KB
8.63 KB

A)

Should ControllerBase make use of RedirectDestinationTrait?

Looking for core classes that extends ControllerBase ( just one level deep - so there will be more. Plus more in contrib.)
I see 44 classes only 2 of which make use of the RedirectDestinationTrait ( see below for the full list)

Many of these controllers, I suspect, could benefit from the trait but that is a different story, for another issue.
For the moment I am going with the minimum change to get the patch clean and so have removed the dependency injection from
ForumController and PathController - they now use the trait..

I have put the original question (#61.2) in the issue summary - to highlight the possibility.

B) Fixed failing test.

C) Fixed up user.api.php - sorry it took so long :)

I am at the end of the time I can spend on this tonight.... I am expecting testbot to pass everything.

I need more time to double check I have address everybodies issues...

grep -R 'extends ControllerBase' core/* | grep '\.php:'
NB this grep pick up 13 test classes which are not counted in the 44 count.

core/lib/Drupal/Core/Entity/Controller/EntityListController.php:class EntityListController extends ControllerBase {
core/modules/aggregator/src/Controller/AggregatorController.php:class AggregatorController extends ControllerBase {
core/modules/aggregator/tests/modules/aggregator_test/src/Controller/AggregatorTestRssController.php:class AggregatorTestRssController extends ControllerBase {
core/modules/block/src/Controller/BlockAddController.php:class BlockAddController extends ControllerBase {
core/modules/block/src/Controller/BlockController.php:class BlockController extends ControllerBase {
core/modules/block_content/src/Controller/BlockContentController.php:class BlockContentController extends ControllerBase {
core/modules/book/src/Controller/BookController.php:class BookController extends ControllerBase {
core/modules/comment/src/Controller/AdminController.php:class AdminController extends ControllerBase {
core/modules/comment/src/Controller/CommentController.php:class CommentController extends ControllerBase {
core/modules/comment/tests/modules/comment_test/src/Controller/CommentTestController.php:class CommentTestController extends ControllerBase {
core/modules/config/tests/config_test/src/ConfigTestController.php:class ConfigTestController extends ControllerBase {
core/modules/config/tests/config_test/src/SchemaListenerController.php:class SchemaListenerController extends ControllerBase {
core/modules/config_translation/src/Controller/ConfigTranslationController.php:class ConfigTranslationController extends ControllerBase {
core/modules/config_translation/src/Controller/ConfigTranslationListController.php:class ConfigTranslationListController extends ControllerBase {
core/modules/config_translation/src/Controller/ConfigTranslationMapperList.php:class ConfigTranslationMapperList extends ControllerBase {
core/modules/contact/src/Controller/ContactController.php:class ContactController extends ControllerBase {
core/modules/content_translation/src/Controller/ContentTranslationController.php:class ContentTranslationController extends ControllerBase {
core/modules/dblog/src/Controller/DbLogController.php:class DbLogController extends ControllerBase {
core/modules/editor/src/EditorController.php:class EditorController extends ControllerBase {
core/modules/field_ui/src/Controller/EntityDisplayModeController.php:class EntityDisplayModeController extends ControllerBase {
core/modules/forum/src/Controller/ForumController.php:class ForumController extends ControllerBase {
core/modules/help/src/Controller/HelpController.php:class HelpController extends ControllerBase {
core/modules/history/src/Controller/HistoryController.php:class HistoryController extends ControllerBase {
core/modules/locale/src/Controller/LocaleController.php:class LocaleController extends ControllerBase {
core/modules/menu_link_content/src/Controller/MenuController.php:class MenuController extends ControllerBase {
core/modules/menu_ui/src/Controller/MenuController.php:class MenuController extends ControllerBase {
core/modules/node/src/Controller/NodeController.php:class NodeController extends ControllerBase implements ContainerInjectionInterface {
core/modules/path/src/Controller/PathController.php:class PathController extends ControllerBase {
core/modules/quickedit/src/QuickEditController.php:class QuickEditController extends ControllerBase {
core/modules/search/src/Controller/SearchController.php:class SearchController extends ControllerBase {
core/modules/shortcut/src/Controller/ShortcutController.php:class ShortcutController extends ControllerBase {
core/modules/shortcut/src/Controller/ShortcutSetController.php:class ShortcutSetController extends ControllerBase {
core/modules/system/src/Controller/AdminController.php:class AdminController extends ControllerBase {
core/modules/system/src/Controller/DbUpdateController.php:class DbUpdateController extends ControllerBase {
core/modules/system/src/Controller/EntityAutocompleteController.php:class EntityAutocompleteController extends ControllerBase {
core/modules/system/src/Controller/Http4xxController.php:class Http4xxController extends ControllerBase {
core/modules/system/src/Controller/SystemController.php:class SystemController extends ControllerBase {
core/modules/system/src/Controller/ThemeController.php:class ThemeController extends ControllerBase {
core/modules/system/src/CronController.php:class CronController extends ControllerBase {
core/modules/system/src/FileDownloadController.php:class FileDownloadController extends ControllerBase {
core/modules/system/tests/modules/entity_test/src/Controller/EntityTestController.php:class EntityTestController extends ControllerBase {
core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php:class ErrorTestController extends ControllerBase {
core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php:class FormTestController extends ControllerBase {
core/modules/system/tests/modules/pager_test/src/Controller/PagerTestController.php:class PagerTestController extends ControllerBase {
core/modules/system/tests/modules/router_test_directory/src/TestContent.php:class TestContent extends ControllerBase {
core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php:class SessionTestController extends ControllerBase {
core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php:class SystemTestController extends ControllerBase {
core/modules/system/tests/modules/theme_test/src/ThemeTestController.php:class ThemeTestController extends ControllerBase {
core/modules/taxonomy/src/Controller/TaxonomyController.php:class TaxonomyController extends ControllerBase {
core/modules/toolbar/src/Controller/ToolbarController.php:class ToolbarController extends ControllerBase {
core/modules/tracker/src/Controller/TrackerPage.php:class TrackerPage extends ControllerBase {
core/modules/tracker/src/Controller/TrackerUserRecent.php:class TrackerUserRecent extends ControllerBase {
core/modules/tracker/src/Controller/TrackerUserTab.php:class TrackerUserTab extends ControllerBase {
core/modules/update/src/Controller/UpdateController.php:class UpdateController extends ControllerBase {
core/modules/update/tests/modules/update_test/src/Controller/UpdateTestController.php:class UpdateTestController extends ControllerBase {
core/modules/user/src/Controller/UserController.php:class UserController extends ControllerBase {
core/modules/views_ui/src/Controller/ViewsUIController.php:class ViewsUIController extends ControllerBase {

willzyx’s picture

Good we're green!

  1. +++ b/core/modules/user/src/Plugin/views/field/Link.php
    @@ -8,12 +8,14 @@
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    was introduced by the patch but it is not used and can be removed

  2. +++ b/core/modules/book/src/Form/BookAdminEditForm.php
    @@ -14,6 +14,7 @@
    +use Drupal\Core\Routing\RedirectDestinationTrait;
    

    was introduced by the patch but it is not used and can be removed

  3. +++ b/core/modules/locale/locale.pages.inc
    @@ -121,5 +121,5 @@ function template_preprocess_locale_translation_last_check(array &$variables) {
    +  $variables['link'] = \Drupal::l(t('Check manually'), new Url('locale.check_translation', array(), array('query' => \Drupal::service('redirect.destination')->getAsArray())));
    
    +++ b/core/modules/system/tests/modules/common_test/src/Controller/CommonTestController.php
    @@ -89,7 +89,7 @@ public function jsAndCssQuerystring() {
    +    $destination = \Drupal::service('redirect.destination')->getAsArray();
    

    Should be \Drupal::destination()->getAsArray();

  4. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Routing\RedirectDestinationTrait;
    
    @@ -17,6 +18,8 @@
    +  use RedirectDestinationTrait;
    +
    
    +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
    @@ -63,7 +63,7 @@ public static function preRenderCompactLink($element) {
    +        'query' => \Drupal::destination()->getAsArray()
    
    @@ -71,7 +71,7 @@ public static function preRenderCompactLink($element) {
    +        'query' => \Drupal::destination()->getAsArray(),
    

    The trait was added but is unused

  5. +++ b/core/lib/Drupal/Core/Routing/RedirectDestinationTrait.php
    @@ -0,0 +1,75 @@
    +   * @see \Drupal\Core\Routing\UrlGeneratorInterface::getAsArray() for
    

    another time my fault :P should be @see \Drupal\Core\Routing\RedirectDestinationInterface::getAsArray()

Should ControllerBase make use of RedirectDestinationTrait?

In my opinion yes, controllers could benefit from the trait.. but given that at the moment only few controllers make use of the redirect.destination service and it can be easily accessed using RedirectDestinationTrait, I do not think that this decision is a priority. We could discuss and decide this in an other issue

martin107’s picture

Assigned: martin107 » Unassigned
Issue summary: View changes
FileSize
49.64 KB
8.63 KB

1) Fixed.
2) Fixed.
3) Fixed.
4) Fixed.
5) Fixed.

Should ControllerBase make use of RedirectDestinationTrait?

Sure lets do this.... I've updated the issue summary to highlight the decision...

[ Full disclosure: As an out of scope change I put the ControllerBase traits into alphabetical order. ]

Sorry ... next time I will clear any review issues directly in the next patch -That created chaos and too many problems has to be pointed out twice. :(

I am unassigning ...I will still jump on any issues like a dog with a bone ... but I think this is ready :)

martin107’s picture

FileSize
6.05 KB

The correct interdif

willzyx’s picture

Issue summary: View changes

Added Beta phase evaluation

willzyx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Martin thanks for the hard work in this issue.
Looks ready to go to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: replace-2448605-65.patch, failed testing.

willzyx queued 65: replace-2448605-65.patch for re-testing.

martin107’s picture

Status: Needs work » Needs review

Looks like testbot had no access to git://git.drupal.org/project/drupal.git

retesting

Status: Needs review » Needs work

The last submitted patch, 65: replace-2448605-65.patch, failed testing.

martin107’s picture

Assigned: Unassigned » martin107

Looks like we have a namespace clash,

#2455083: Open redirect fixes from SA-CORE-2015-001 need to be ported to Drupal 8

Introduced a new method SystemTestController::getDestinaiton() which conflicts with a method of the same name in out newly introduced trait.

I will try and get to this over the weekend.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
49.69 KB
17.72 KB

Naming things is hard ..... it was much less work that I though to modify patch directly and nobble the 30 instances of the name so am happy to do it again if the new name offends anyone

getDestination is a generic name
getDestinationProperty also exists
getRedirectDestination exists.

in the end I adopted getDestinationArray()

Status: Needs review » Needs work

The last submitted patch, 74: replace-2448605-74.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
750 bytes
50.24 KB

Missed one.

willzyx’s picture

personally I'd like to leave the name of the method in the trait unchanged and change rather the name in the test class (SystemTestController::getDestination)
getDestination() is much more clear and clean from the DX perspective.. what do you think about it?
however I think that the patch can be marked RTBC anyway

willzyx’s picture

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

Status: Reviewed & tested by the community » Needs review

I agree I would prefer to keep getDestination() also

1) it was the smallest change
2) it keep the similarity with \Drupal\migrate\Row::getDestination - which returns an array
3) if we have to, we should place any compromised name in tests to keep core pure.

BUT I was being a cautious bunny ....

Looking for the uses of SystemTestController::getDestination() I see only the service "system_test.get_destination' - a service which is unused!!!!

So as far as I could see we don;t need it at all --- which made me think that I was missing something and modifying a security related patch without full understanding seemed stupid. That is an easy way to break things. Perhaps something in contrib is using the name and changing the name is equivalent to a API change...

@dawehner can you comment on the uses of SystemTestControler::getDestination() I see you worked on #2455083: Open redirect fixes from SA-CORE-2015-001 need to be ported to Drupal 8

tim.plunkett’s picture

I disagree. Just because migrate named it confusingly, doesn't mean we should too. I would expect getDestination to return a string, getDestinationArray is very clear.

martin107’s picture

Prefer is a small thing ...getDestinationArray() is fine by me ... :)

I think we have a green viable patch ... does that mean back to RTBC?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Indeed

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

I'm not convinced that getDestinationArray is that great. But I guess it is ok since it is mapping to getAsArray.

We need to update https://www.drupal.org/node/2448603 with this patch.

+++ b/core/lib/Drupal/Core/Routing/RedirectDestinationTrait.php
@@ -0,0 +1,75 @@
+use Drupal\Core\Routing\RedirectDestinationInterface;

Not used.

willzyx’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record updates
FileSize
2.18 KB
50.19 KB

removed unused import and changed record updates https://www.drupal.org/node/2448603
Back to RTBC assuming it passes

martin107’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
648 bytes
50.17 KB

The interdiff from #84 has some extra code in it... but when I recreate the interdiff locally everything is correct..

I have a last minute trivial issue to remove .... when I pass the new trait through phpcs I get a warning about additional text not being allowed on a line with a @see tag

The text seemed superfluous so I removed it.

PS Cool, the change record looks good :)

willzyx’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 issue summary update

So the beta evaluation is missing a key piece for normal tasks - why should we be making this change now. All change is at least a tiny bit disruptive because it causes patches to need rerolls and takes time to review. I think we should be making this change because we have code that mocks this in unit tests which breaks the contract about phpunit tests not polluting the global namespace...

+++ b/core/modules/views/src/Tests/Plugin/views/field/EntityOperationsUnitTest.php
@@ -156,15 +163,3 @@ public function testRenderWithoutDestination() {
-namespace {
-
-if (!function_exists('drupal_get_destination')) {
-  function drupal_get_destination() {
-    return array(
-      'destination' => 'foobar',
-    );
-  }
-}
-
-}

+++ b/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
@@ -323,12 +323,3 @@ protected function assertViewResultCommand(ViewAjaxResponse $response, $position
-namespace {
-  // @todo Remove once drupal_get_destination is converted to autoloadable code.
-  if (!function_exists('drupal_static')) {
-    function &drupal_static($key) {
-      return $key;
-    }
-  }
-
-}

Not good... and great that this patch removes them.

martin107’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Altered the issue summary:-

1) Mentioned the disruption to the issue queue.
2) Added the "polluting the global namespace" argument

willzyx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 85: replace-2448605-85.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
799 bytes
50.17 KB

Rerolled.

willzyx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 91: replace-2448605-91.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

.

martin107’s picture

Status: Needs work » Needs review
FileSize
50.22 KB

trivial reroll - a use statement above one of the changes was modified.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

martin107’s picture

Issue tags: -Needs reroll
alexpott’s picture

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

Needs yet another re-roll

martin107’s picture

Status: Needs work » Needs review
FileSize
49.54 KB

Rerolled

cause

FieldEditForm.php has gone away -- so we no longer need to patch-up changes to that file.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 99: replace-2448605-99.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

.

willzyx’s picture

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

rerolled

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/tests/src/Unit/Plugin/views/field/EntityOperationsUnitTest.php
    @@ -44,7 +43,15 @@ public function setUp() {
    -    $this->plugin = new EntityOperations($configuration, $plugin_id, $plugin_definition, $this->entityManager);
    +
    +    $this->plugin = $this->getMockBuilder('Drupal\views\Plugin\views\field\EntityOperations')
    +      ->setConstructorArgs([$configuration, $plugin_id, $plugin_definition, $this->entityManager])
    +      ->setMethods(array('getDestinationArray'))
    +      ->getMock();
    +    $this->plugin->expects($this->any())
    +      ->method('getDestinationArray')
    +      ->willReturn(['destination' => 'foobar']);
    +
    

    why are we now mocking this class? This is the class under test so it should be the original if possible? Please add a comment or switch it back to testing the original.

  2. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php
    @@ -65,6 +65,13 @@ class CustomPageExceptionHtmlSubscriberTest extends UnitTestCase {
       protected $customPageSubscriber;
    

    now the defined protected variable is unused and should be removed.

martin107’s picture

Assigned: Unassigned » martin107

Please add a comment or switch it back to testing the original.

I think that was me .... One sec I am looking into it. :)

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review

#104.1

Yep - I was refactoring away this problematic code from the foot of the EntityOperationsUnitTest.php

namespace {

if (!function_exists('drupal_get_destination')) {
  function drupal_get_destination() {
    return array(
      'destination' => 'foobar',
    );
  }
}

A summary of #87 is alexpott pointing out that committing this issue is mostly unjustifiable at this point in the release cycle,
except for this cleanup ....

We are mocking out just one function and leaving the rest of the class intact, so from a certain perspective we are mocking this otherwise hard to test \Drupal::destination(); call within RedirectDestinationTrait.

  protected function getRedirectDestination() {
    if (!isset($this->redirectDestination)) {
      $this->redirectDestination = \Drupal::destination();
    }

    return $this->redirectDestination;
  }

As always consensus is important - in this case it become in effect "should we move this issue to something > 8.1 ?"

Would you like me to supply comment in the code?

#104.2

Changes made by this patch

BEFORE customPageSubsciber was assigned by this line

-    $this->customPageSubscriber = new TestCustomPageExceptionHtmlSubscriber($this->configFactory, $this->aliasManager, $this->kernel, $this->logger);

where TestCustomPageExceptionHtmlSubscriber was defined at the bottom of the file.

AFTER customPageSubscriber is now assigned a few lines later by

  $this->customPageSubscriber = new CustomPageExceptionHtmlSubscriber($this->configFactory, $this->aliasManager, $this->kernel, $this->logger, $this->redirectDest
 

Have I missed something the variable is still used as before...

    $this->customPageSubscriber->onException($event);
klausi’s picture

FileSize
48.93 KB

klausi opened a new pull request for this issue.

klausi’s picture

FileSize
1.83 KB

Sorry, #104.2 was my fault, I was searching for the variable in a wrong way. Looks good.

I fixed the mocking for EntityOperationsUnitTest. The trait allows us to set and mock the service, and this is what we should do.

Interdiff attached

martin107’s picture

Thanks - that does remove an imperfection I introduced :)

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Ok, since you approve of this change and the rest of the patch also looks good I think we can set this to RTBC now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d48d9c9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed d48d9c9 on 8.0.x
    Issue #2448605 by martin107, willzyx, tim.plunkett, klausi, dpopdan:...

Status: Fixed » Closed (fixed)

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