Problem/Motivation
All of the functionality of actions is contained within Drupal\Core\Action
The action.module
only provides the UI.
Proposed resolution
As this module is just providing a UI in the same way as menu_ui.module
, views_ui.module
, and field_ui.module
, rename to action_ui.module
Remaining tasks
#2811663: Rename Action module to Actions UI in the UI and in comments
#3067299: Move actions migrations and tests to system module
#3022401: Remove useless config action.settings.recursion_limit
#3022399: Move action_views_form_substitutions() out of actions module
#2815379: Move message, goto, and email action plugins to Drupal\Core\Action
User interface changes
N/A
API changes
N/A, the action module does not provide an API.
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#86 | interdiff_85-86.txt | 1.76 KB | ravi.shankar |
#86 | 2083649-86.patch | 59.22 KB | ravi.shankar |
#85 | 2083649-85.patch | 59.14 KB | Munavijayalakshmi |
#83 | 2083649-83.patch | 57.74 KB | ajaypratapsingh |
#82 | 2313164-82.patch | 57.74 KB | ajaypratapsingh |
Comments
Comment #1
tim.plunkettWe had some stale code as well, its nice to clean this up.
Comment #3
alexpott#1: action_ui-2083649-1.patch queued for re-testing.
Comment #4
tim.plunkettBtw, no upgrade path is needed because action.module was new to D8 (previously includes/actions.inc).
Rerolled now #2082499: Uninstalling action module removes actions created by the user module is in.
Comment #6
tim.plunkett#4: action_ui-2083649-4.patch queued for re-testing.
Comment #7
tim.plunkettTagging.
Comment #8
tim.plunkettComment #9
star-szrTagging for reroll.
Comment #10
roma7 CreditAttribution: roma7 commented1: action_ui-2083649-1.patch queued for re-testing.
Comment #12
roma7 CreditAttribution: roma7 commented4: action_ui-2083649-4.patch queued for re-testing.
Comment #14
roma7 CreditAttribution: roma7 commentedI am going to do a reroll of https://drupal.org/comment/7837905#comment-7837905
Comment #15
roma7 CreditAttribution: roma7 commentedComment #16
roma7 CreditAttribution: roma7 commentedComment #17
longwaveComment #18
roma7 CreditAttribution: roma7 commentedComment #23
roma7 CreditAttribution: roma7 commentedComment #24
roma7 CreditAttribution: roma7 commentedComment #26
roma7 CreditAttribution: roma7 commentedComment #28
roma7 CreditAttribution: roma7 commented26: action_ui-2083649-26.patch queued for re-testing.
Comment #29
dawehnerLet's us write a proper issue summary here.
This hook is indeed not called anymore.
Comment #30
roma7 CreditAttribution: roma7 commentedComment #31
alexpottI'm now wondering if this rename is actually correct - with fields_ui and views_ui it is possible to configure views and fields and then disable or completely remove these modules. However with actions this would not be the case since there are plugins in the module and any configured action that relied on them would not work once the module is disabled.
Comment #32
tim.plunkettUgh, that's my fault.
EmailAction, GotoAction, and MessageAction used to be in system module, and I moved them to action module.
That was wrong. They should be moved to \Drupal\Core (preferred), or even back to system (if needed).
Comment #33
sun#1008166: Actions should be a module explains why all of this code was moved out of system.module.
Separating the UI into a action_ui.module would make sense to me — would be in line with #2085243: Rename Menu module into Menu UI module
If it's possible to move the API functionality into a
Drupal\Core
component, then that might make sense...However, part of the reason for action.module was that not every Drupal site has a need for configurable actions functionality. → A module can be kept disabled.
Lastly, it looks like there's still a bug in the action synchronization code... Apparently, the active config directory of my local dev site contains plenty of action config files, but I'm confident that I never configured any actions. I remember that I stumbled over that problem in the original issue already.
Comment #34
tim.plunkettWe still don't have entity types in Drupal\Core, still haven't revisited it.
Unlike some of the other module move/renames, this is a perfect 1:1 move, as all of the non-UI functionality is already moved out.
Also, the config you weren't expecting came from user_user_role_insert().
Comment #35
dawehner26: action_ui-2083649-26.patch queued for re-testing.
Comment #37
daffie CreditAttribution: daffie commentedI think this good for DX. But it is to late to get it into 8.0.0. I think that it is not a good idea to move it to 8.1.x for BC reasons. So lets move this one to 9.x-dev
Comment #38
jian he CreditAttribution: jian he commentedComment #39
xjmComment #40
catchThis could be done in 8.x with a bc layer, it'd be horrible but it would allow 9.x to just drop that bc layer rather than introducing a new bc break.
Comment #41
dawehnerA first minor step, but maybe important for many people would be to change the label/description of it actually describe what its doing. In this case we would describe that it provides the UI for what is provided by system module by default.
Comment #42
naveenvalechaAgreed with daniel, The first step would be the do the changes in UI and in comments. Filed a followup for it #2811663: Rename Action module to Actions UI in the UI and in comments
Then internals like Forms, Controllers in one issue then @api classes
Comment #47
jibranComment #48
andypostIt is not clear from summary why rename makes sense except same-looking naming pattern
Comment #49
tim.plunkettComment #50
andypost@tim.plunkett IS not actually true because all configurable actions are defined in system module
So probably better to wait for #2815379: Move message, goto, and email action plugins to Drupal\Core\Action
Comment #51
tim.plunkettAgreed, thought that had landed
Comment #52
andypostNow unblocked, is there reason yo split this into #2811663: Rename Action module to Actions UI in the UI and in comments
Comment #53
andypostAdded child issue for views hook remains #3022399: Move action_views_form_substitutions() out of actions module
Comment #54
andypostAnd one more child issue #3022401: Remove useless config action.settings.recursion_limit
Comment #56
andypostComment #57
andypostFiled last blocker (2) for rename #3067299: Move actions migrations and tests to system module
Comment #58
MixologicRenaming core modules has the side effect of causing core's own dependency checking to fail if a contrib module already had a dependency on the module.
There's about 14 modules on drupal.org that have a dependency on drupal:action, for example: https://git.drupalcode.org/project/commerce_bulk/blob/8.x-1.x/commerce_b...
Or
https://git.drupalcode.org/project/commerce_inventory/blob/8.x-1.x/comme...
Is there some way to provide a BC layer for renames like this?
Comment #59
tim.plunkettI'm not sure how best to handle that BC. But both of those modules (and any others) can remove that dependency. The Action system is in
Drupal\Core\Action
and is not separate from drupal/core, so that declaration is pointless. (I checked, neither module alters the Action UI directly, which is what the module provides)Comment #60
andypostChecked the modules in #58 they wrongly depends on actions instead 1) depends on views for "vbo"
Comment #61
MixologicThose were just example modules. Maybe the other 12 are similar.
What happens to a module that declares a dependency on 'drupal:action' if that module gets renamed on an upgrade? Does that module no longer run? Does it uninstall itself? What does the modules page do with that situation? Or, does it likely not matter and pretty much any module with a dependency on action already has it satisfied, regardless of whether or not the action module is enabled?
Comment #62
vacho CreditAttribution: vacho at Skilld commentedComment #64
andypost2 blockers left, today accepted #3022401: Remove useless config action.settings.recursion_limit
PS issues are ordered in summary
Comment #65
karthik.arumugam CreditAttribution: karthik.arumugam commentedI have applied #26 patch to branch 8.9.x. Please review.
Comment #66
karthik.arumugam CreditAttribution: karthik.arumugam commentedComment #68
jungleAbout BC, how about keeping a simplest action module in core, adding actions_ui as a dependency with a .info.yml file and an empty .module, then marking it deprecated.
Comment #69
andypost@jungle no reason, action entity in system module but should be part of core (when it was created there was no ability to provide entities in core namespace)
Comment #71
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#65 successfully apply on 9.2. Just running test cases for 9.2 on #65
Comment #72
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedFixed CS issues.
Comment #75
Gábor HojtsyIf we still want to do this in Drupal 10 despite the composer problems outlined by @Mixologic in #58 and #61?
If so, the parent should be changed to #3118154: [meta] Deprecate dependencies, libraries, modules, and themes that will be removed from Drupal 10 core by 9.4.0-beta1 and work on the Drupal 9 branch should be done to prepare for this. Then another issue should be opened parented to #3213858: [META] Remove deprecated modules and themes from the Drupal 10 branch so the actions module can be removed in Drupal 10.
Comment #76
andypostComment #77
penyaskitoWhat about a mix of #68 + an update_hook for enabling
action_ui
ifaction
is enabled?We can create an issue on those 14 modules for removing the dependency.
Then for D10, we can just remove
action
module. They will need to take care of the dependencies before/when D10 is released, but that would be acceptable (they probably will need to update theircore_version_requirement
at least anyway)Comment #78
andypostGood idea and right time, ++ to #77
Comment #79
andypostFor #77
Comment #80
andypostThe only one issue left #2811663: Rename Action module to Actions UI in the UI and in comments
Comment #82
ajaypratapsingh CreditAttribution: ajaypratapsingh at Srijan | A Material+ Company for Drupal India Association commentedrerolled patch
Comment #83
ajaypratapsingh CreditAttribution: ajaypratapsingh at Srijan | A Material+ Company for Drupal India Association commentedSorry previous patch #82 have a wrong patch name(issue no.)
Comment #84
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound for Valuebound commentedComment #85
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound for Valuebound commentedRectified all custom command failed errors, except following
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig core/modules/action_ui/
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/src/ActionListBuilder.php
--------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------
117 | WARNING | Variable $build inside unset call is undefined.
--------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/lib/Drupal/action_ui/ActionListController.php
----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
118 | WARNING | Variable $build inside unset call is undefined.
----------------------------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/lib/Drupal/action_ui/Tests/ConfigurationTest.php
-------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
34 | ERROR | [x] Visibility must be declared on method "testActionConfiguration"
-------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/action_ui.info.yml
-----------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------
1 | WARNING | "core_version_requirement" property is missing in the info.yml file
-----------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/action_ui.views_execution.inc
------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------
13 | WARNING | Do not use check_plain() on string literals, because they cannot contain user provided text
------------------------------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/tests/src/Functional/ConfigurationTest.php
------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------
10 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
------------------------------------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/tests/src/Unit/Menu/ActionLocalTasksTest.php
---------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------
15 | ERROR | [x] Doc comment short description must start with a capital letter
---------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/tests/fixtures/update/drupal-8.action-3022401.php
-------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------
35 | ERROR | unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.
-------------------------------------------------------------------------------------------------------------------------------------------
FILE: /var/www/html/vb/drupal-3299800/core/modules/action_ui/tests/action_form_ajax_test/action_form_ajax_test.info.yml
-----------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------
1 | WARNING | "core_version_requirement" property is missing in the info.yml file
-----------------------------------------------------------------------------------------------------------------------
Time: 722ms; Memory: 12MB
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig core/modules/comment/lib/
FILE: /var/www/html/vb/drupal-3299800/core/modules/comment/lib/Drupal/comment/Tests/CommentActionsTest.php
----------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------
33 | ERROR | [x] Visibility must be declared on method "testCommentPublishUnpublishActions"
53 | ERROR | [x] Visibility must be declared on method "testCommentUnpublishByKeyword"
----------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------
Time: 93ms; Memory: 10MB
Comment #86
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAddressed Drupal CS issues of patch #85 and removed needs reroll tag.
Comment #90
quietone CreditAttribution: quietone at PreviousNext commentedThe Actions Module is now approved for removal from core, #3266458: [Policy] Deprecate Action (UI) module in D10 and move to contrib in D11. Doing further work, such as renaming it are now outdated.
Thank you to everyone who worked to complete this issue!