Updated: Comment #0
Problem/Motivation
The function is already duplicated in views.module's NodeContextualLinksTest and config_translation.module is duplicating it a third time currently. This will happen in contrib a lot more.
Proposed resolution
Move the function to WebTestBase. Most test cases won't use it, but I still think it makes sense to provide it there. Since we don't have traits, there's nowhere else to put it.
Remaining tasks
Review patch.
User interface changes
None.
API changes
Only an API addition: WebTestBase::renderContextualLinks
Related Issues
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | move-2113679-12.patch | 4.34 KB | manuel garcia |
| #9 | 2113679-9.patch | 134.69 KB | rasikap |
| #1 | 2113679-1.patch | 4.44 KB | tstoeckler |
Comments
Comment #1
tstoecklerHere we go.
Comment #2
dawehnerLooks solid as it is, even I am wondering whether this is used that often that it really is worth to copy to the base class.
Comment #3
tstoecklerYeah, with the new drupalPost() I must admit I thought the same thing when rolling this. config_translation.module still had the hold version at the time, just like views.module which is what made this wort it. We could just as well inline it, I don't feel strongly about that.
Comment #4
tstoecklerComment #6
jhedstromThis no longer applies.
Instead of moving to
WebTestBase, perhaps moving to a trait that can be used as needed would be better?It's also in use in several more places now:
Comment #7
ashishdalviWe will take this issue on Drupal Mumbai code sprint today.
Comment #8
rasikap commentedComment #9
rasikap commentedAdding rerolled patch.
Following files were resolved for conflicts :
1. core/modules/contextual/src/Tests/ContextualDynamicContextTest.php
2. core/modules/node/src/Tests/Views/NodeContextualLinksTest.php
3. core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
Comment #10
tstoecklerThat was a bad merge, as
WebTestBasehas moved in the meantime.I also agree with #6, using a trait for this makes a lot of sense. We should also consider the ongoing conversion to
BrowserTestBase, so would be great to have some input from @klausi/@dawehner/... if that is affected by this or vice versa.Comment #12
manuel garcia commentedReroll of #1
Comment #25
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Reviewing the patch against 10.1.x and see NodeContextualLinksTest no longer has a function for contextualLinks. Is this still a valid task since only ContextualDynamicContextTest has the function now?
Comment #27
smustgrave commentedSince there hasn't been a follow up going to close for now. If still a valid task please reopen.