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
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 |
Comment | File | Size | Author |
---|---|---|---|
#108 | interdiff.txt | 1.83 KB | klausi |
#107 | 2448605.patch | 48.93 KB | klausi |
#103 | replace-2448605-103.patch | 49.56 KB | willzyx |
#99 | replace-2448605-99.patch | 49.54 KB | martin107 |
#95 | replace-2448605-95.patch | 50.22 KB | martin107 |
Comments
Comment #1
martin107 CreditAttribution: martin107 commentedJust 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.
Comment #2
martin107 CreditAttribution: martin107 commentedSo 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.
Comment #3
tim.plunkettScrolling through quickly, most of those places need to use dependency injection instead of \Drupal::service
Comment #4
martin107 CreditAttribution: martin107 commentedAh that is great news,
I was too timid to do that given we are in beta ... I am happy to do this properly.
Comment #5
martin107 CreditAttribution: martin107 commentedComment #6
martin107 CreditAttribution: martin107 commentedTaking 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.
Comment #7
tim.plunkettComment #10
tim.plunkettTypo
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.
Comment #11
martin107 CreditAttribution: martin107 commentedMy time to work on this this evening is suddenly cut short....bugger
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
Comment #13
dpopdan CreditAttribution: dpopdan commentedComment #14
dawehnerLet's test the patch.
Comment #16
martin107 CreditAttribution: martin107 commentedOk so starting from #13... I have converted 5 classes to the new 'inject the service' way of doing things.
more daily...
Comment #18
martin107 CreditAttribution: martin107 commented1) 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.
Comment #20
martin107 CreditAttribution: martin107 commentedRefactoring 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.
Comment #22
willzyx CreditAttribution: willzyx commentedWe can use
\Drupal::destination()
instead of\Drupal::service('redirect.destination')
Comment #23
martin107 CreditAttribution: martin107 commentedOk 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.
Comment #24
martin107 CreditAttribution: martin107 commentedThis should bring the error count down :)
Comment #26
martin107 CreditAttribution: martin107 commentedConverted another 5 classes.
Comment #28
martin107 CreditAttribution: martin107 commented1) 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
well I am getting identical errors popping up in another unrelated issue.
https://www.drupal.org/node/2448915#comment-9709727
Comment #30
martin107 CreditAttribution: martin107 commentedUserAdminListingTest now passes.
Comment #32
martin107 CreditAttribution: martin107 commentedForumTest now passes.
Comment #34
martin107 CreditAttribution: martin107 commentedLooking 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.
Comment #35
martin107 CreditAttribution: martin107 commentedOk 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. :)
Comment #36
martin107 CreditAttribution: martin107 commentedIn 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... ]
Comment #38
martin107 CreditAttribution: martin107 commented1) 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.
Comment #40
martin107 CreditAttribution: martin107 commentedI 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.
Comment #42
willzyx CreditAttribution: willzyx commentedthis should fix the fail
Comment #44
willzyx CreditAttribution: willzyx commentedehm..for real now
Comment #45
willzyx CreditAttribution: willzyx commentedI had forgotten to attach the interdiff
Comment #46
martin107 CreditAttribution: martin107 commentedJust 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.
Comment #47
willzyx CreditAttribution: willzyx commentedI'm wondering whether it would be better to create RedirectDestinationTrait, instead of continuing on this path
from #2134513: [policy, no patch] Trait strategy
and I think this might be the case
Comment #48
martin107 CreditAttribution: martin107 commentedthanks 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.
Comment #49
martin107 CreditAttribution: martin107 commentedSorry 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.
Comment #50
martin107 CreditAttribution: martin107 commentedLooking 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.....
Comment #51
willzyx CreditAttribution: willzyx commentedI'm not sure.. If we create RedirectDestinationTrait and insert it in
Drupal\Core\Form\FormBase
and inDrupal\user\Plugin\views\field\Link
we can (almost) avoid changing constructors. It would simplify and make the code more clean imhoAttached a draft of RedirectDestinationTrait
Comment #52
martin107 CreditAttribution: martin107 commented@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.
Comment #53
willzyx CreditAttribution: willzyx commentedyes.. use RedirectDestinationTrait simplifies a lot this task
Why don't move RedirectDestinationTrait directly in
\Drupal\user\Plugin\views\field\Link
?Comment #55
martin107 CreditAttribution: martin107 commentedA) 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.
Comment #56
martin107 CreditAttribution: martin107 commentedComment #57
willzyx CreditAttribution: willzyx commentedgreat work. Some observations:
isn't needed since BookAdminEditForm extends FormBase
$this is not accessible in static context
and
and
Should be \Drupal::destination()->getAsArray();
my bad.. should be "The redirect destination service."
Why don't move RedirectDestinationTrait directly in \Drupal\user\Plugin\views\field\Link?
Comment #59
martin107 CreditAttribution: martin107 commentedLast 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.
Comment #61
dawehnerThis looks pretty great!
What is the reason to add the trait without using it at all?
What about providing this as part of the ControllerBase as well?
Comment #62
martin107 CreditAttribution: martin107 commentedIt is unusual for the review to point out bug fixes ( #57 pointed out the bugs before testbot could report them! ) -- thank you both...
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.
Comment #63
martin107 CreditAttribution: martin107 commentedA)
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 {
Comment #64
willzyx CreditAttribution: willzyx commentedGood we're green!
was introduced by the patch but it is not used and can be removed
was introduced by the patch but it is not used and can be removed
Should be \Drupal::destination()->getAsArray();
The trait was added but is unused
another time my fault :P should be @see \Drupal\Core\Routing\RedirectDestinationInterface::getAsArray()
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
Comment #65
martin107 CreditAttribution: martin107 commented1) Fixed.
2) Fixed.
3) Fixed.
4) Fixed.
5) Fixed.
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 :)
Comment #66
martin107 CreditAttribution: martin107 commentedThe correct interdif
Comment #67
willzyx CreditAttribution: willzyx commentedAdded Beta phase evaluation
Comment #68
willzyx CreditAttribution: willzyx commentedMartin thanks for the hard work in this issue.
Looks ready to go to me.
Comment #71
martin107 CreditAttribution: martin107 commentedLooks like testbot had no access to git://git.drupal.org/project/drupal.git
retesting
Comment #73
martin107 CreditAttribution: martin107 commentedLooks 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.
Comment #74
martin107 CreditAttribution: martin107 commentedNaming 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()
Comment #76
martin107 CreditAttribution: martin107 commentedMissed one.
Comment #77
willzyx CreditAttribution: willzyx commentedpersonally 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
Comment #78
willzyx CreditAttribution: willzyx commentedComment #79
martin107 CreditAttribution: martin107 commentedI 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
Comment #80
tim.plunkettI 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.
Comment #81
martin107 CreditAttribution: martin107 commentedPrefer is a small thing ...getDestinationArray() is fine by me ... :)
I think we have a green viable patch ... does that mean back to RTBC?
Comment #82
tim.plunkettIndeed
Comment #83
alexpottI'm not convinced that
getDestinationArray
is that great. But I guess it is ok since it is mapping togetAsArray
.We need to update https://www.drupal.org/node/2448603 with this patch.
Not used.
Comment #84
willzyx CreditAttribution: willzyx commentedremoved unused import and changed record updates https://www.drupal.org/node/2448603
Back to RTBC assuming it passes
Comment #85
martin107 CreditAttribution: martin107 commentedThe 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 :)
Comment #86
willzyx CreditAttribution: willzyx commentedBack to RTBC :)
Comment #87
alexpottSo 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...
Not good... and great that this patch removes them.
Comment #88
martin107 CreditAttribution: martin107 commentedAltered the issue summary:-
1) Mentioned the disruption to the issue queue.
2) Added the "polluting the global namespace" argument
Comment #89
willzyx CreditAttribution: willzyx commentedComment #91
tim.plunkettRerolled.
Comment #92
willzyx CreditAttribution: willzyx commentedBack to RTBC
Comment #94
dawehner.
Comment #95
martin107 CreditAttribution: martin107 commentedtrivial reroll - a use statement above one of the changes was modified.
Comment #96
dawehnerBack to RTBC
Comment #97
martin107 CreditAttribution: martin107 commentedComment #98
alexpottNeeds yet another re-roll
Comment #99
martin107 CreditAttribution: martin107 commentedRerolled
cause
FieldEditForm.php has gone away -- so we no longer need to patch-up changes to that file.
Comment #100
dawehnerBack to RTBC
Comment #102
dawehner.
Comment #103
willzyx CreditAttribution: willzyx commentedrerolled
Comment #104
klausiwhy 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.
now the defined protected variable is unused and should be removed.
Comment #105
martin107 CreditAttribution: martin107 commentedI think that was me .... One sec I am looking into it. :)
Comment #106
martin107 CreditAttribution: martin107 commented#104.1
Yep - I was refactoring away this problematic code from the foot of the EntityOperationsUnitTest.php
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.
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
where TestCustomPageExceptionHtmlSubscriber was defined at the bottom of the file.
AFTER customPageSubscriber is now assigned a few lines later by
Have I missed something the variable is still used as before...
Comment #107
klausiklausi opened a new pull request for this issue.
Comment #108
klausiSorry, #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
Comment #109
martin107 CreditAttribution: martin107 commentedThanks - that does remove an imperfection I introduced :)
Comment #110
klausiOk, since you approve of this change and the rest of the patch also looks good I think we can set this to RTBC now.
Comment #111
alexpottCommitted d48d9c9 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.