Problem/Motivation
(original report motivation)
The default action on /admin/content
is "Delete selected content". It's really easy to push "Apply" button by mistake, thinking it will apply the filter, because of the button title, and because it's positioned at the bottom of the form, typically where people expect to look for submit buttons. (This actually happened to me today, and I've been using Drupal for 8 years)
Steps to reproduce
See screenshots.
Proposed resolution
MR to review !4583
Proposal:
- Store the order of the actions in the Views Bulk Form handler configuration (actually, the View config entity)
- Provide an upgrade path to set the order config in each Bulk Form field but preserve BC. On existing sites the order should be preserved even the "dangerous" actions are on top. Then site builders might decide to manually change that.
- For new sites provide a default order that is safe. Specifically, the "Delete" action should be the last in the list avoiding incidents.
A little history>
Initially, adding a weight to action config entities has been tried. However, as is mentioned in #152, this is not the correct approach as an action (read a config entity) may appear on several views and the site builder may need different sort orders on each view. Read the #152 comment for more background.
Remaining tasks
None.
User interface changes
Bulk Form admin form
Before:
The checkboxes let you select which actions will be available, but you cannot control the order.
After:
The form element used to select the actions is converted from checkboxes
to draggable table
. The table allows both, actions selection and action reordering.
Views with Bulk Form
The actions are reordered for new sites on /admin/content
, /admin/content/comment
, /admin/content/comment/approval
, /admin/content/media
with the "Delete" action as last option.
Before:
After:
API changes
None.
Data model changes
New actions_order
option for Bulk Form handler.
Release notes snippet
Use drag and drop in Views bulk form field configuration form to define order which will be used to expose the actions in the bulk forms.
Comment | File | Size | Author |
---|---|---|---|
#236 | 2381293-nr-bot.txt | 90 bytes | needs-review-queue-bot |
#231 | 2381293-nr-bot.txt | 10.49 KB | needs-review-queue-bot |
#229 | 2381293-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-2381293
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
grendzy CreditAttribution: grendzy commentedComment #2
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedI produced a few alternatives and put them in the issue summary. Disclaimer: The statements of usage (frequent/infrequent) in the alternatives are assumptions based on my contact with side editors of sites build by both myself and by third parties. Feel free to replace it with facts.
Variation 1: D7-like
Pros: Familiar to D7 users
Cons: Publish is executed without confirmation
Variation 2: Safe default
Pros: 1. Safe default. Content state is not changed; 2. Freedom to choose any order for the remaining actions; 3. All frequently used operations on the top of the list.
Cons: 1. Clicking Apply will with the default result in a (friendly) (error) message; 2. New UX pattern for core
Variation 3: Safe first
Pros: Non-destructive content operation as default option
Cons: 1. Default operation is executed without warning; 2. Less frequently used operations are placed on top.
As a side note, I think the Save option should be removed. But perhaps we should create a follow-up issue for this.
Comment #3
yoroy CreditAttribution: yoroy at Wunder commentedAgree the current situation is not a good default. I don't think we have to come up with really new ideas here. Matching it to what we have in D7 is a good enough fix I'd say.
Comment #4
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedI investigated the code behind this drop down list.
The items in the select list are bulk operation actions and can be configured in the Content view (admin/structure/views/view/content). Similar select lists are used in the Comment admin and User admin views. The order of user actions list (admin/people) needs some love too.
The items in the list are Action plugins (e.g. Drupal\node\Plugin\Action\DeleteNode). They are sorted by their action plugin ID (e.g. node_delete_action). The action plugin does not have a weight to be sorted by.
I see a few ways to fix it:
- Add a weight to the Action plugin and and sort them by it. This includes a modification of the schema of the Action plugin.
- Let Views add a weight to the action plugins, perhaps first hard coded, later with a sort option in the Views interface.
- Reverse the current sort order. Unpublish will become the first option.
Comment #5
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedAn additional solution to explore after discussion with dawehner:
- Add the property 'category' to the Action plugin and sort by category first, by ID next. Block plugin already has a 'category'.
Comment #6
milosetfrs CreditAttribution: milosetfrs commentedassigning to me.
Comment #7
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #8
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedI am working on it.
Comment #9
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedJust few changes are done in order to get safer drop down list (similar as Alternative 2: Safe default).
- None - is added as first item in the list (default) and validation of option value. Reverse the current sort order can be done if it is needed.
Using weight or category gives more flexibility but I don't know what will be affected with changes should be done.
Comment #10
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedComment #12
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedAdd some tests.
This looks like:
Comment #13
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedSecond solution, same as Alternative 3: Safe first.
Added weight to Action, changed schema, added default weight values and overridden function getBulkOptions.
Comment #15
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedCode is a bit simpler.
I can't solve fails in tests. When default weight values are set in core\modules\node\config\install\system.action.node_*_action.yml, get these fails. Guess that system.action.* is proper changed.
Some advice?
Comment #16
slashrsm CreditAttribution: slashrsm as a volunteer commentedComment #18
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedTrying to fix patch.
Comment #19
toniteof CreditAttribution: toniteof at MD Systems GmbH commented2381293-18.patch looks like:
Comment #20
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedAdd image for comment #12.
Comment #21
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedPatch looks ok to me... We would need to add update hook which would load all actions, set appropriate weights on them and save them.
Comment #22
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedThanks @slashrsm
Add hook update and tests.
Comment #23
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedLet's set weight for the actions that are not in the list above to some sane default. 0 maybe?
Comment #24
toniteof CreditAttribution: toniteof at MD Systems GmbH commented0 is set as the default weight for other actions.
Comment #25
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedComment #26
toniteof CreditAttribution: toniteof at MD Systems GmbH commentedA little bit better documentation.
Comment #27
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedTested issue on simplytest.me. Looks good on frontend.
Attached image
Delete is moved to bottom. Further if we don't want to change the order, it's good to move to RTBC
Comment #28
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedComment #29
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented+1 for RTBC
Comment #30
catchDoes/should the weight show up in the config entity form?
Comment #31
yoroy CreditAttribution: yoroy commentedThanks for working on this @toniteof!
@catch: I think this screenshot means the answer is no?
I don't think they have to be made reorderable(?), a fixed order keeps is better here, just like "Quit" is always the bottom one in desktop apps etc.
Comment #32
swentel CreditAttribution: swentel commentedI also think exposing them would be a bit weird since it would be hard to change them one by one. It probably would be better to use the draggable list builder to order them all at once, but that's maybe for a follow up ?
Comment #33
Bojhan CreditAttribution: Bojhan as a volunteer commentedYhea, lets fix the bug - and then do the reodering in refinements?
Comment #34
catchYep completely agreed we shouldn't show it unless we eventually do a draggable list, and we shouldn't do the draggable list here, just wanted to check :)
Back to RTBC. Due to the update/config schema change this should just go into 8.1.x
Comment #35
alexpottSo this is changing an interface by adding a method which is a total BC break and we can't do even in 8.1.x. There is an issue to discuss whether entities should have interfaces see #2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release.
See for a discuss of this see the most recent comments on #306662: Add redirect option to site-wide contact forms
Comment #36
catchIt doesn't change the interface, only the base class, but it probably should pending those discussions. There's also comments on #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation.
Comment #39
NikitaJain CreditAttribution: NikitaJain commentedTested and verified #26 patch (2381293-26.patch) on dev-8.3.x. Its working fine and "Delete Content" has moved to the bottom.
Screenshots attached.
Comment #40
webchickBecause of the solution chosen, seems like it'd be good for this to have Framework Manager sign-off.
However, it would be awesome to see this fixed because yuck!
Comment #41
alexpottThis update needs to be in system module because all actions now have weight not just the ones provided by node and the node module is not required.
Comment #44
kbentham CreditAttribution: kbentham commentedI have rebase the patch from #26 against 8.4.3. I also moved the update hook from the node module to the system module.
Comment #45
kbentham CreditAttribution: kbentham commentedI also rebased the patch against 8.5.x.
Comment #47
manningpete CreditAttribution: manningpete at American Medical Association commentedComment #50
kbentham CreditAttribution: kbentham at Palantir.net for American Medical Association commentedI have rerolled the patch in #45 against 8.6.x and fixed the syntax errors.
Comment #52
kbentham CreditAttribution: kbentham at Palantir.net for American Medical Association commentedI have fixed the syntax errors thrown by the last patch.
Comment #54
kbentham CreditAttribution: kbentham at Palantir.net for American Medical Association commentedI have updated the tests for 8.6.
Comment #56
kbentham CreditAttribution: kbentham at Palantir.net for American Medical Association commentedUpdate the delete action plugin names.
Comment #57
kbentham CreditAttribution: kbentham at Palantir.net for American Medical Association commentedFix the user_add_role_action test failures and rebase against the newest version of 8.6x.
Comment #58
benjifisherI think the problem is that
&=
does not do logical AND. It does bitwise AND, returning an int instead of a bool.Let's see what the testbot thinks of my version of the patch.
Comment #60
benjifisherHere is a minimal patch that (I hope) fixes the failing tests. I did the following:
Drupal\node\Plugin\views\field\NodeBulkForm::getBulkOptions()
from the patch in #26.weight: 0
tocore/modules/media/config/optional/system.action.*.yml
I am not sure that the new test should be added to the
setUp()
method in the node module'sBulkFormTest
. This means the test is executed twice. Why not add it totestBulkForm()
?Do we want to update
getBulkOptions()
in NodeBulkForm or in the parent class? The update function is in the system module (as requested in Comment #41) and weights are added to all (?) the core actions, but so far we only use those weights in the node module.Do we want to reorder the actions in the Media bulk-options list as part of this issue, or is that out of scope? (Can we make it in scope by changing the issue title?)
Comment #63
benjifisherOops, I misnamed my interdiff.
The previous patch actually changed the bulk actions form. It turns out that another test assumes that "Delete content" is the default action. This patch takes care of that.
Comment #65
dqdDoes it still apply on 8.7.x-dev ?
Comment #66
benjifisher@diqidoq:
Thanks for having a look. No, the patch does not apply on 8.7.x, so it needs a re-roll. This is probably a good Novice task, so I will add the appropriate tags.
Comment #67
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commentedComment #68
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commentedComment #70
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commentedComment #72
benjifisher@govind.maloo:
Thanks for taking this on!
If you follow the link to the (failing) test results, and scroll to the bottom, you will find a link labeled "1 coding standards message". If you click to expand that, it points you to one line that should use the short array syntax. I think fixing that problem is a suitable Novice task.
It looks as though the failing tests were added as part of #2788777: Allow a site-specific profile to be installed from existing config, which explains why the patch in #63 did not cause them to fail. This may be a case where the tests have to be updated. In fact, I think the three tarballs under
core/tests/fixtures/config_install/
need to be updated for the config changes in this issue. Feel free to work on that if you are interested, but it is also OK to fix the coding-standard issue and remove the Novice tag.Comment #73
vacho CreditAttribution: vacho at Skilld commentedResolved the novice issue with array syntax.
Comment #74
vacho CreditAttribution: vacho at Skilld commentedthe patch can apply, so no need to reroll
Comment #75
andypostbetter to refactor it as
uasort($options, [SortArray::class, 'sortByWeightElement']);
and add touse
the classnameComment #76
andypostComment #77
benjifisherThe patch for #70 was a tricky reroll. When I applied the patch from #63 and merged with the 8.7.x branch, there was a merge conflict because ActionResourceTestBase.php in the Rest module was more-or-less moved to ActionResourceTestBase.php in the System module. I think the reroll was done properly, adding a
weight
key togetExpectedNormalizedEntity()
in the new location.Good work, @govind.maloo!
I have attached an interdiff comparing the patches in #70 and #73. The patch in #73 makes exactly the change it says it does. Thanks, @vacho!
I am updating the remaining tasks.
Comment #78
Berdirwhy specific to node? we have e.g. media actions too and there's more in contrib. The names are fairly unified, so instead of having the complete name for node, we could for example do a partial match for "delete" and so on.
Comment #80
danrodComment #78:
why specific to node? we have e.g. media actions too and there's more in contrib. The names are fairly unified, so instead of having the complete name for node, we could for example do a partial match for "delete" and so on.
Having the same issue with comments as well, the patch #73 works perfect for nodes, but the default action on "/admin/content/comment" is still "Delete comment".
I will try to rewrite this patch to make it work with comments as well.
Comment #81
yoroy CreditAttribution: yoroy at Roy Scholten commentedThat would be great @danrod. This has been a long standing usability bug, lets fix it :)
Comment #82
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedAdding a straight re-roll for 8.8.x.
However, I would like to point out also @danrod's suggestion. From what I see, if the overwrite of the method happens on the base class instead of the node override, it should work out of the box.
As for actions from other entity types, from what I can understand, in general, people in this ticket don't want delete to be first, which mostly is the case due to the letter 'D'. Other thing I noticed here is that 'Save' comes even later so we want to have the 'Delete' as the pre-last case and save as last one.
We can have the update hook set every action that contains the 'delete' string to -1, the 'save' string to 0 and all the rest to a default value -2 (since Drupal tends to count from 0 backwards when it comes to weight).
All related actions in Core share these two strings.
Then, for the other cases, e.g. comments and media, separate tickets can be opened so that the specific order is decided and checked in terms of usability.
Comment #83
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedSetting to NR so that tests run.
Comment #85
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedAdding my approach on this for review. Will fix the tests as well.
Comment #86
andypostInteresting idea to move sorting, BC is a question
Comment #88
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented@andypost there were no tests up to now ensuring the order of the actions and the system update ensures that the existing actions will receive a default value.
The only case where a problem could happen is in the sorting when the entity does not have the weight set. However,
SortArray
has a backup for this as thesortByWeightElement
calls forsortByKeyInt
which starts withSo the order will not change in that case.
What other BC break might occur?
Also, attaching an attempt to fix the tests.
Comment #89
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI am moving the update path to the post updates since we are making changes to config entities. Hook updates are meant for changes like altering databse tables.
Also, it was annoying when I was trying to apply the changes to our project. We are still running 8.7 and this is meant for 8.8 and was marked as update 8802. That could cause a issues.
Comment #90
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI am also adding a condition where I skip cases where the weight is already set. The reason is that if someone comes across this patch before it is merged in core, they will probably find it because they already have the need for weights and possibly different weight than the one offered here.
However, the post update of the system module might run later than that user's update.
This is to ensure that the update will not overwrite any other values.
Comment #91
pfrenssenThis should be:
We also should move this documentation to the
ActionConfigEntityInterface
and replace this one with{@inheritdoc}
.An earlier review in this issue rejected the method in the interface as a B/C break, but this was before we nailed down the B/C policy. Adding methods to interfaces is already allowed since Drupal 8.1.x (ref Backwards compatibility policy).
Updating the actions of the node module should not happen in the system module but instead in a post update inside the Node module. This will break in sites that do not have the Node module enabled.
We also need a similar update hook in all other modules that offer a delete action, such as the Comment module, the Media module and the User module.
This was suggested by @berdir in #2381293-78: Actions reordering on views bulk forms but I don't like this, it relies too much on magic for my taste. Checking if the machine name of an action contains the word 'delete' is not reliable.
Even in the core actions this doesn't work, for example the action to delete a user account is
system.action.user_cancel_user_action
- it doesn't contain the word 'delete'.We should not try to babysit action defined in contrib and try to autofix them by guessing their functionality by the machine names. Let's just reliably fix the actions that are provided by core in scope of this patch.
Contrib modules that consider it important to reorder their actions will be able to set their own weights in a post update hook once this gets in.
Comment #92
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented1. You are right, done.
2. We are not updating 'node' actions in the system hook. We are hardcoding the names of the node actions and if the name of the loaded action is included in the list, we are setting a different weight.
Since this is the first application, I do not think there is a problem performing this in the system update. However, I will move it to the node and make sure that the update does not conflict with each other and the order doesn't matter.
The only thing I don't get is what do you propose about the rest of the actions. We should update explicitly actions for all other entity types in this ticket or apply a generic default value (-2) and leave it for a follow up?
3. No strong opinion here. Reverted the changes.
Attaching an attempt for the above.
Comment #93
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedComment #95
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedOops, it seems that the
$config_factory->loadMultiple
is loading Immutable config objects so I cannot use this to load them all.Anyway, there are only 8 of them so it is not an issue to load them one by one.
Comment #96
claudiu.cristeaThis will change the order of the list for existing sites and I think this should never happen. New sites will get the new order but existing sites behaviour should be preserved. Then the site builders, acknowledging the new weight feature, will be able to make a decision and, if case, will reorder the actions. That being said, what we need as (post)update is to ensure a
weight: 0
for existingsystem.action.*
configs.Why
-2
? The weight can be positive, zero or negative. The smaller will get on top. So, I think we need to put all existing configs on0
except the "delete" ones which will get a positive value. This would cover also existing contrib/custom actions that will not update their config YAML files, becauseSortArray::sortByWeightElement()
considers the lack ofweight
key asweight === 0
. See also the previous comment. And we can move the whole (post_update logic insystem
module as we iterate over allsystem.action.*
configs and those config entities are defined bysystem
module.Also the update path needs tests.
Comment #97
claudiu.cristeaAlso, this is doing the job but it's not quite the right way to update a config value. Using
::set()
is the way:EDIT: but we have to use the helper for updating config entities https://www.drupal.org/node/2949630
Comment #98
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented@claudiu.cristea In general I continued the request in the ticket. The "Delete" should not be the first action. That is why I continued on the philosofy of the patches before mine. Indeed it is kinda breaking the BC and it is not like it is hard for sites to adapt to their will accordingly.
I am providing a patch that switches back to the idea of the default weight being 0. Also, updating the title of the issue so that it matches your remarks.
I am also fixing one more case of an action created with a null weight so that the missing expected weight does not make the tests fail.
Also updated the system post update function to use the new approach for updating config entities.
I am not hidding the earlier patches to keep a space for the rest to raise an argument if needed.
Comment #99
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedOops, forgot the status change.
Comment #100
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedUgh... Please, ignore the interdiff... I rerolled the patch to the latest develop but compared it to the previous branch (as a branch) and the result was weird.
Uploading the actual interdiff.
Comment #101
claudiu.cristeaThe interdiff is weird. Also I see that all actions have now weight 0. Shouldn't an action, such as
system.action.node_delete_action
have a higher weight so that it sinks to the bottom of the list? New sites will get the new order. Existing sites without config management will work as they do now. Existing sites that are managing config are owning the site config in their sync directory. Exporting the new config is part of an update, so it should be an effect of a decision.We don't need the type hint, we can strict type in the closure signature. And I think we should use the interface.
A more accurate check would be to test if is null:
$action->get('weight') === NULL
.Comment #102
andypostI think it makes sense to add weight to plugin annotation, this way delete action will get default "high" weight
PS: here is example #2994748: Make a way for Help Page Sections to be ordered
Comment #103
claudiu.cristea@andypost, I'm not so sure. I think the logic of having the split between plugin and config is clear. The plugin provides the logic while the config is customizing a specific implementation of the plugin. Meaning we may use an action plugin in more than one place but with different customizations (different config entities). The
weight
, looks to me, more belonging to customization (i.e. config entity), than the plugin business logic. In a view we may want the "Delete..." action on the bottom of the list, on other circumstances we my want it on other position. It provides the same business logic but in different contexts.Comment #104
pfrenssenThe way I read the suggestion from @andypost it is that the annotation weight is the _default_ weight, to be set initially when the config is created.
This is in line with some of the other annotation values that are also being stored in the config when it is created: like the ID, label and entity type.
Having the default value in the annotation makes it possible for action plugin developers to provide a default weight without having to ship an install / update hook.
Comment #105
claudiu.cristea@pfrenssen @andypost, the plugin annotation providing a "default weight" sounds interesting. That meaning the config can override that value. If a config is created and no
weight
has been specified, then will default to the plugin default weight. This is my understanding. That being said, I think, on annotation, it should bedefault_weight
, not simply 'weight". Then the post upgrade path will have to set0
to keep the actual order.Comment #106
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedOk, then how do we proceed? do we go with @andypost's suggestion or do we wait for him to answer? Btw, I am also in favor of what @claudiu.cristea initially said. The linked issue is providing the weight property to the
HelpSection
plugin. Then in theHelpController
the weight is being passed to the section output directly, atAs @claudiu.cristea pointed out, the helper section plugin is a plugin that holds the information displayed in the help page and holds explicitly information on the section information.
On the contrary, an
Action
is a plugin holding information about, well, an action. An action is not sortable or is having any meaning for weight. The weight doesn't really make sense outside the scope of the form because it is the only place where we sort them. It is not like we are going to use the plugin manager to load all actions and we do not want the delete action to be the first one.What we do display on the form is an
Action
config entity instead. This config entity is meant to be listed and shown in the UI so it makes sense for it to have a weight in case it needs custom sorting.I scavenged a bit and found more config entities that do such a thing:
explicitly declare the weight as a default value in the protected property as
\Drupal\user\Entity\Role
is not using the defaultprotected $weight = 0;
way but instead uses a::preSave
method to fill in the default value if left empty and then there is the case of\Drupal\search\Entity\SearchPage
which is having it created in the::postCreate
method which however was based on the https://www.drupal.org/node/2004756 which was closed without action in which Berdir points out the direct assignment to the property or the::preCreate
method for setting default values (points out in general, not in comparison for the config entities).The config entities above, if I am not mistaking, are not based on some plugin but their purpose is pretty clear.
To give you another example, let's say that in the future we create some entity that performs more than one actions in core. That entity would somehow reference more than one actions. In that case, a weight property in the plugin itself would make much more sense since it would matter in the sequence that they are executed. If we set the
$weight
property to the plugin now, we merely declare thatAction
plugin now are tied to the UI of Drupal as it carries a property that affects its appearance.I am not going to refactor the patch any longer until we have a final decision on that. @andypost can we have your thoughts on this as well? Also @claudiu.cristea and @pfrenssen.
Comment #107
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI have discussed this with @claudiu.cristea about if the decision is final and was concluded that we will go with the default plugin. I still have concerns on this but still, here is a candidate patch.
Comment #108
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedAlso, I am restoring the previous title as according to the #101, this is still needed. Also, remarks from #101 are met.
Comment #110
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedSome fixes for the tests.
Comment #111
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedIt turns out that the suggestion from @andypost has caused the bulk actions for media entities to also change order. Since this is the case, I added a test similar to the one added for the nodes.
Comment #112
pfrenssenI really like how this is looking now. Some small remarks:
This sounds a bit strange, how about "The default weight for new actions.".
It should also be
@var int
to be in line with our coding standards. It is not allowed to add(optional)
to a property declaration.Since this is public this should be documented on the
Drupal\Core\Action\ActionInterface
and replaced with{@inheritdoc}
here.I would also change the documentation to be clear that this returns the default weight of the action.
This is missing a period at the end. Let's change it to
The default weight the action will have in the UI.
to be entirely clear what this is about.This test is not about node operations but media library operations.
The weight should be initialized in the constructor, not in the
::preSave()
method. Otherwise if we call$action->getWeight()
on a newly created, unsaved Action entity we are gettingNULL
back which is in violation of the interface (which declares that this returns an integer).This test is missing documentation.
This test is wrong, it should instead test that the correct weight (as set in the plugin annotation) is returned immediately after creation. The
$action->getWeight()
method should never return NULL but should always return the correct value.Comment #113
andypostI don't wanna detail it more but what about using priority instead of
weightComment #114
andypostComment #115
pfrenssenI'm not sure that priority would be better than weight. I think weight is more in line with how we commonly name this in Drupal.
Priority is mostly used to change the order of execution, while weight is used for the order in the UI.
Comment #116
BerdirAgreed that we should stick to weight, which we use almost everywhere in Drupal, Symfony uses priority. The main difference is IMHO not about execution order vs order in UI, but that the values are inverted. high order => end of the list, high priority => start of the list.
Comment #117
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented@pfrenssen thanks for the review.
I removed that. I got it from the
\Drupal\filter\Annotation\Filter
's weight property actually.In general, the only case where that can help is the upgrade path. Loading actions and having them with NULL weight is easier to check if they need update. However, I think you are right on wanting the weight to be set on the constructor.
I am providing an attempt to fix all remarks.
Comment #118
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedSo, in terms of the
__constructor
/::presave
remark I had changed it but I am reverting back to the original way for the main reason that the weight is being retrieved by the plugin, but during the\Drupal\system\Entity\Action::create
method does not have the plugin_id as a mandatory property.This causes a crash when you try to create an Action object without the plugin ID passed in from the beginning.
To keep this in the constructor, I see two ways, either handle the case where the plugin is not set yet, which might cause the object to be stored without a weight, or we will include it both in the constructor and the presave which is a redundant piece of code.
I have updated the docblock to show that it can return NULL if the weight has not been set yet.
Comment #120
pfrenssenI had a look at the cases where this can happen and if there was maybe another way to solve it. One example is that an action entity is created in
user_user_role_insert()
. This is indeed a use case where the entity is created in a standard way without passing the weight. The developer expectation here is that the weight should get the default value. In this flow it is indeed a good solution to use the::preSave()
to set the value.I have one final small suggestion:
This code block is no longer needed and can be removed.
Also it looks like the upgrade path test still needs to be written.
Comment #121
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI fixed a small underscore that I added to test the update manually, cleaned up the code and provided an update test.
Also, thanks for the guidance :D @claudiu.cristea also helped with pinpointing the update test base class :)
Comment #122
andypostIt looks great, now needs change record
Comment #123
pfrenssenAwesome work! Assigning for writing the change record.
Comment #124
pfrenssenCreated draft change record: https://www.drupal.org/node/3083509
I reviewed the changes in #121 and they look good. RTBC from me.
Comment #125
pfrenssenComment #126
alexpottI think this code is unnecessary. \Drupal\system\Entity\Action implements EntityWithPluginCollectionInterface which means all the plugin values are copied to the config entity automatically.
\Drupal\Tests\system\Kernel\Action\ActionTest::testDefaultWeight()
passes with this code removed.Comment #127
claudiu.cristeaWe should init this to 0. Annotations without
weight
will take this value. I know it's already init inActionBase
but I would do it also here.Comment #128
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedFixed the remarks from #126 and #127.
Comment #130
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedIt seems that the tests fail.
@alexpott it was actually a false positive. The
::assertEquals
does a==
comparison and considers0
equals toNULL
.There is a similar issue that explains this a bit already at https://github.com/sebastianbergmann/phpunit/issues/1717#issuecomment-11...
I tried to find a place where values were copied (or filled) over by the plugin but could not find anything.
I changed the check to
::assertSame
which does not safecast anything and does a type comparison as well.Everything seems to be returning to normal like this.
Bottom line, it seems that the test was wrong but the
::preSave
was right.Comment #131
alexpottSo I've taken a long look at this patch and I think we need to step back a bit. At the moment weight is being set on the action plugin annotations and then also saved as a root property on the configuration entity. But when it is being saved on the config entity is being saved outside of the plugin configuration. That's not really correct it should be part of the plugin configuration. If it was, then the logic in \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() would ensure that the value on the plugin and configuration are in-sync. This isn't working because weight is not being saved as part of
\Drupal\system\Entity\Action::$configuration
(which is where plugin config goes).One really big question is should weight even be configurable? It doesn't have to be - it could be something defined in the annotation and leave it at that.
If we are going to make it configurable then the following things need to be addressed. However I think adding it to the annotation and therefore part of the plugin's abilities could come first and then we could make it part of configuration in a separate patch.
This doesn't need to be here.
I don;t understand why we're doing this. How has a value already been fixed?
As this is the first time actions have weight we should be getting the weight from the annotation. If we get this right re-saving the config entity will be enough to pull the default config in the plugin system.
These configurations shouldn't need to be changed. They are part of an update test and so should be as it was at the time when the update function was written.
Comment #132
BerdirJust some quick inputs:
* A bit confused about the annotation vs plugin configuration explanation from @alexpott, only default config is added to the plugin configuration, and if it's not configuration then why should it be in there?
* Keep in mind that only some action plugins are configurable, most aren't, so we can't rely on adding something to the default configuration and it then just working. The UI currently also only allows to configure/add configurable action plugins, but I think we should change that.
* At the same time, if action plugins are actually configurable, then being able to configure weights as a user definitely makes sense, e.g. with #2797583: Dynamically provide action plugins for every moderation state change, you want the publish action to be shown above the archive action.
Comment #134
claudiu.cristeaReroll for 8.8.x.
Comment #141
pfrenssenOpened a MR with the patch rebased on 9.4.x.
Comment #142
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commented@alexpott about 2. and 3., we are doing both for BC (or at least this is how we thought it).
For 2, this is because, as soon as this is accepted, a user interested in this might create their own update hook in order to set their desired weight. Since the system default weight setup takes place in a post update, any update hook or configuration synchronization before that might already set the values.
For 3, it is even more simple. According to @claudiu.cristea's comment #96, we want to by default not change the current behavior in existing sites. That means that all sites should get an equal weight so that the order is preserved.
Question to all, should I also update the 8.8.x branch? is this going to be committed to 8.8.x too?
Edit: 1 seems to be fixed in 9.4.x and last point does not apply as the actions don't seem to exist anymore in 9.4.x.
Comment #143
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedComment #144
claudiu.cristeaNW, see the MR.
Comment #145
claudiu.cristeaRegarding #131.2:
@alexpott, well, in theory it doesn't. But, practically, a lot of sites were already using this patch and site builders did set weights on their actions. I think it doesn't cost us to much to add an
if (...)
. However, that needs to be handled inside the action configs update closure, as I proposed in https://git.drupalcode.org/project/drupal/-/merge_requests/1670#note_69909Comment #146
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedComment #147
claudiu.cristeaThere's a misspelled word, see https://www.drupal.org/pift-ci-job/2314338:
Comment #148
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedComment #149
claudiu.cristeaFew more remarks and an architectural observation: Should the plugin provide the default weight as annotation or as an internal property (with setter/getter)?
I'm opting more for annotation as this is a static, non-changing, value and fits well with the annotations scope.
EDIT: If we go with annotation, then it should be renamed as
default_weight
.Comment #150
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedPeople I have an issue and require your intervention.
The above code works fine for PHP8.0 but fails for PHP7.4. That is because we do not sort by label as a secondary sort and the actions (apart from the weight) are simply presented as they come from the database. I don't know if something related to this changed in PHP8.0 because it seems to come like this from the storage. How should we proceed with this?
Comment #151
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedComment #152
claudiu.cristea@idimopoulos, thank you for moving on with this, looks good.
However, I'm not anymore sure about the architectural approach...
Proposal
Comment #153
claudiu.cristeaHere's a Proof Of Concept (no interdiff as is a fresh approach)
Still needs:
Regarding the UI: Unfortunately Drupal core lacks a tableselect with draggable rows. I went with an approach used also in text formats: A list of checkboxes and, below, the re-rdering draggable table.
Comment #154
claudiu.cristeaAnd seems to belong to Views module now.
Comment #156
claudiu.cristeaFix tests
Comment #157
claudiu.cristeaFix also the JS test
Comment #160
claudiu.cristeaAnother try...
Comment #161
claudiu.cristeaFixing the dependency to actions.
Still needs:
Comment #162
claudiu.cristeaAdding missing dependencies in
views.view.*.yml
files.Comment #163
Gauravvvv CreditAttribution: Gauravvvv at Srijan | A Material+ Company for Drupal India Association commentedRe-rolled patch #162, Attached interdiff for same.
Comment #164
claudiu.cristeaFix test, CS, etc...
Comment #165
claudiu.cristeaAdded test coverage.
Comment #166
claudiu.cristeaComment #167
claudiu.cristeaFix tags.
Comment #169
claudiu.cristeaFixed DefaultConfigTest test. Still need to figure out how to fix Umami profile failures.
Comment #170
claudiu.cristeaMove Media actions in config/install as now they are dependencies for other module config.
Comment #171
claudiu.cristeaComment #172
benjifisherI am fixing a typo in the issue summary.
I plan to look at this issue in the weekly usability meeting (maybe this week). Thanks, @claudiu.cristea, for the clear issue summary, especially the "User interface changes" section. If you think of any other usability questions that we should consider, then please add them there.
Comment #173
claudiu.cristea@benjifisher, Thank you. It seems that I was able to overcome the UX issue. I've managed to merge the actions selection with the actions ordering in a single widget. However, it needs some CSS love.
Comment #174
benjifisherThat is good news!
Does this issue still need usability review? Does it need an accessibility (a11y) review?
Do you plan to work on the CSS yourself or should we try to recruit a front-end developer to help?
Comment #175
claudiu.cristea@benjifisher, CSS is not my strongest point. It would be nice if a frontend dev can step in.
Not sure about usability but yes to accessibility.
Other aspects:
Removing a leftover.
An important question here is: Do we really need the dependencies management here?
Because now, with this patch, when an action gets deleted, the bulk form fields are removed from the view and the view is disabled. This looks very aggressive. Views doesn't seem to handle nice dependency removals. But if we remove the dependency management, on an action removal, the dead action ID will continue to be stored in the View but will not harm in any way, as only existing actions are loaded. Then on the next view save, the dead action IDs are removed.
Comment #176
markconroy CreditAttribution: markconroy at Annertech commentedI think if you add the draggable library as a dependency to you patch, you should get the correct alignment, same as we use when dragging blocks up/down on the Admin > Structure > Blocks page.
Comment #177
claudiu.cristeaThank you @markconroy for guiding me on the CSS issue. It would be great if you can review, at least, the CSS part. Here are the CSS fixes (I'm updating also the IS). I think UX review is not needed anymore.
Comment #178
claudiu.cristeaAs per #175, I think we should not do dependency management. See #175 for details. I've undone all the work regarding calculating dependencies and "on dependency removal".
I've changed my mind let's keep Needs usability review
Comment #181
claudiu.cristeaFix theme library override tests.
Comment #182
markconroy CreditAttribution: markconroy at Annertech commentedRemember, we need to support IE11 for now, so should not use
width: min-content
.I'd suggest we use
width: 1.5rem
or something like that.Comment #183
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedThis will be very disappointing (it was to me) but while discussing with @claudiu.cristea I remembered that in the beginning, we thought it properly. When me, @claudiu.cristea and @pfrenssen started working on this, it was to have weight only as part of the plugin. I guess we lost track later in the QA when it was proposed to be configurable because at that point, we should have thought that in multiple views, we cannot have multiple weights storing the weight in the configuration.. And I have the feeling that we had discussed this in the past but lost track.
Anyway, I think strongly that the latest approach from @claudiu.cristea is the correct approach. I still think that if we want a "Delete" action to not be first by default, we can still add the plugin non-configurable weight, so that actions are initialized in the view but this is not really needed with configurable weight within the view.
Only thing I am thinking of is that in D7 I remember there are modules offering sub-options. I don't know if this patch should care about this. An example is below:
Of course, there are solutions for this, facets is doing it already by dynamically adding the settings in another container lower in the page. Maybe something like that could apply for this too.
Comment #184
benjifisherI have read some of the recent comments, but not looked at the code. I have two questions.
First, this issue may be suffering from scope creep. Is there a workable solution that makes something other than Delete the default action, without adding per-view configuration?
Second, please confirm my understanding of this, from #175:
This is the expected behavior of the patches in #178, #181, correct?
If so, then I agree with that change. We already have similar behavior with other views dependencies.
Details: I recently removed several unused roles from a site. Any views exposed filter has the option to "remember" the selection based on role. If I remember correctly, every filter on every view stores the list of roles, even if the filter is not exposed. I noticed this when I searched the YAML files for the removed roles. I had to edit each view, and edit each filter, then save, in order to get the view to save without the obsolete roles. (Or I could edit the YAML files directly. I am not saying.)
Comment #185
claudiu.cristea@benjifisher,
Very early, since #4, the scope was clarified to not only provide a default safe order of the actions but to allow actions reordering. We're within the scope of the issue. Initially, this has been achieved by adding a weight on actions but this is not quite correct. I've explained in #152 why.
Yes. Initially, I've implemented calculate dependencies & on dependency removal (see #177 patch), but this creates the following issue: When an action is deleted, the bulk form fields using that action are removed from the view(s) and the view(s) is/are disabled. This is too aggressive. Instead, I prefer to keep the dead references to removed actions in the view. This has no effect as only the existing actions are loaded and exposed. So, dead references are still there but doesn't create any issue. If the field is updated and the view is saved, they are removed from the view storage. Not sure is the same situation as with the roles.
@idimopoulos
True, but that is the VBO contrib. It's a totally different thing. The Drupal core Bulk Field doesn't expose such sub-forms.
Comment #186
claudiu.cristeaFixed #182
Comment #187
benjifisherI am adding some screenshots to the issue summary.
I think a slight drawback is that the tabledrag (when editing the available actions in the view) takes up so much space. The 8 options available with the Standard profile fill up most of the modal window on a fairly large (desktop) screen.
Despite that, I think this issue is a big help.
Comment #188
Devashish Jangid CreditAttribution: Devashish Jangid at Dotsquares Ltd. commentedVerified and tested patch #186.
Patch applied successfully and looks good to me.
Sharing screenshot for the reference.
Moving this ticket to RTBC.
Comment #189
sourabhjainComment #190
benjifisherUsability review
We discussed this issue at #3266551: Drupal Usability Meeting 2022-03-04. That issue will have a link to a recording of the meeting.
One participant mentioned that her clients have sometimes clicked the "Apply" button when they meant to click "Filter". So this issue addresses a real problem, not a theoretical one!
I am setting the status to NW because the Views configuration for Claro needs some work, as this screenshot show:
We also thought it would be a good idea to add a do-nothing option at the top of the list as the default. Something like "- Select -" or whatever the usual label is when a selection is required. There was pretty strong consensus on this.
The last idea we had was to change the default order, grouping opposites together. For example, Publish and Unpublish should be next to each other. This should probably be done in a follow-up issue, unless you would like to work on it here.
Comment #191
benjifisherI forgot to remove the tag for a usability review.
For the record, the participants at the usability meeting were
benjifisher, rkoller, AaronMcHale, andregp, Antoniya, ckrina, guilherme-rabelo, guilherme-rabelo, kimberlly_amaral, victoria-marina, shaal, tmaiochi, worldlinemine
Comment #192
claudiu.cristeaMany thanks for the review
Wouldn’t that be a BC break? Suddenly existing sites will have a new default option when they probably expect there an action. Maybe we can add the no-action text as a new configuration and on empty text we don’t show this empty option. Then we can provide BC for existing sites… But would that be acceptable from a UX perspective? I mean adding more clutter to the config form. If yes, IMHO that is slightly out-of-scope and it would be a good candidate for a follow-up. Any thoughts?
Could anyone with better CSS skills step in? I’m really not the best person.
I think we should do it here because releasing this change as it is will raise other BC problems in the follow-up
Waiting for a UX guidance on the first problem
Comment #193
benjifisherThe question about BC is a good one. I think we do not have to preserve compatibility in this case. According to Drupal 8 and 9 backwards compatibility and internal API policy (backend),
There is more detail on the companion page Drupal 8 and 9 frontend backwards compatibility (BC) policy:
(emphasis added).
Given the strong consensus at the usability meeting, I think this issue is one where we are allowed to make a BC break.
Comment #194
benjifisherLooking at the screenshots in the issue summary, I see that Drupal 7 grouped opposite actions together, as we suggested in #190. I think that going back to the same order as D7 is a reasonable default.
Comment #195
claudiu.cristeaAssigning to work on #190 & Co.
Comment #196
claudiu.cristeaSwitched back to MR https://git.drupalcode.org/project/drupal/-/merge_requests/1670
Comment #197
claudiu.cristeaI've implemented UX requirements from #190
Comment #203
claudiu.cristeaWork moved to !2931
Comment #205
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedComment #206
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #208
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedRebased to latest head and fixed Merge conflict in MR #2931
Comment #209
smustgrave CreditAttribution: smustgrave at Mobomo commentedTried testing MR 2931
I see the reordering feature in the bulk operation view field.
But reordering and checking different items seems to have no baring.
I checked 2 items but all appear.
Comment #211
claudiu.cristea@smustgrave, I don't understand what "no baring" means in this context. Could you, please, explain?
Comment #213
smustgrave CreditAttribution: smustgrave at Mobomo commentedMeant that I couldn't reorder or remove actions with the change.
But is this needed anymore default option is no longer "delete". If still needed definitely will need an issue summary update.
Comment #215
claudiu.cristeaRe #209, #213
@smustgrave ,
Thank you for looking at this.
This is not about removing actions. It's only about reordering of actions on a per-view basis. I've tested again manually and everything works as expected. Also, the tests are a proof that is working correctly.
The IS is explaining all these and doesn't need any change. However, I've tweaked a little the Proposed resolution to make it more clear.
Please re-read the scope and test again. For any issue I'm available here or on Slack. Thank you.
Comment #216
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan none 11.x branch be hidden or noted in IS which MR should be reviewed.
Assumed 4583 as that's the last one but appears to have a test failure.
Applied the MR though and can confirm I can change the order without issue.
Comment #220
claudiu.cristeaHide attached files
Comment #221
claudiu.cristeaAdded MR to be reviewed in IS
Comment #222
claudiu.cristeaA new action has been introduced recently, in 79a07a4e, and that made the test fail. This is ready for review.
Comment #223
smustgrave CreditAttribution: smustgrave at Mobomo commentedThat seemed to address the test failure.
Retested the reordering and that functionality still works.
Comment #224
quietone CreditAttribution: quietone at PreviousNext commentedUpdating credit
Comment #225
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues. I read the IS, the comments, the MR and change record.
Thanks for keeping the Issue Summary up to date. I have moved the 'MR to review' to the 'proposed resolution' section because it is the proposed resolution. ;-)
Like @benjifisher, I am concerned about scope creep on this issue. The direction set in #4 states, "- Let Views add a weight to the action plugins, perhaps first hard coded, later with a sort option in the Views interface." I read that to mean the changes to the Views interface are to be done later. That is also a suitable separation of tasks here. And bring the patch size down to something manageable for a review.
I spotted one nit in the MR (which I did not review) and the grammar in the change record needs work. However, I am not commenting or tagging for those because the scope issue should be settled first.
I am leaving at RTBC and will ask the committers.
Comment #226
claudiu.cristeaThat was more a proposal than setting the issue’s scope. And until #152 a different approach has been considered, which we considered to be functionally wrong, for the reasons expressed in #152. Of course we can separate the UI part in a follow-up but the work is already done here and it benefited from the contributions of UX and fronted devs.
Comment #229
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #230
claudiu.cristeaRerolled
Comment #231
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #232
claudiu.cristeaRerolled
Comment #233
catchThe phpcs job failed.
Comment #234
sourabhjainComment #235
claudiu.cristeaSetting back as RTBC as it was just very minor change in the PHPCS rules added recently
Comment #236
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #237
andypostIt needs merge of
core/tests/fixtures/config_install/multilingual.tar.gz
after #3333401: Pager h4 causes accessibility flag on many pagesComment #238
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedRebased to latest head but need to merge commits, a697018f and 78d40bad - Update /multilingual.tar.gz .
@claudiu.cristea by any chance do you remember what those changes are ?
Comment #239
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commented