shortcut_install() doesn't check config installer syncing flag while editing the seven theme settings.
Update the shortcut_install() so that it only edits config when config installer syncing flag is FALSE.
Review.
Commit.
None.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | 2550357-nr-bot.txt | 85 bytes | needs-review-queue-bot |
| #24 | 2550357-24.patch | 2.1 KB | alexpott |
| #22 | 2550357-nr-bot.txt | 153 bytes | needs-review-queue-bot |
| #13 | fix_shortcut_install-2550357-13.patch | 2.72 KB | jibran |
| #6 | fix_shortcut_install-2550357-6-test-only.patch | 1.17 KB | jibran |
Show commandsStart 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:
changes
Comments
Comment #2
jibranCreated #2550385: Update hook_install documentation for editing configuration as doc follow up.
Comment #3
larowlanconsistent with forum_install()
Comment #4
jibranThank @larowlan.
Comment #5
alexpottIt strikes me that we are missing test coverage. ConfigImportAllTest is the place to do this. We should change the value after this...
And the test the value at the end of the test. Should just be a couple a lines and it'd be nice to have a failing patch to show people what we're on about.
Comment #6
jibranI created this patch but it doesn't fail. Uploading it without the fix. What am I doing wrong here?
Comment #7
jibranNeeds more investigation.
Comment #10
jibran@alexpott any idea how to proceed with this?
Comment #13
jibranWhy do we need to check this?
Why?
system_installis setting the site uuid do we need to check config installer syncing flag there as well?Meanwhile, I have updated the patch. If we should be checking the flag then we should do it for both
shortcut_installandshortcut_themes_installed.Comment #22
needs-review-queue-bot 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 #23
alexpottComment #24
alexpottComment #25
alexpott@jibran - system is a special case. It's always installed and the site UUID is even more special with respect to configuration imports.
Comment #26
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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 #28
acbramley commentedLooks like this is still valid (install hook in shortcut on 11.x still looks roughly the same), needs a reroll into an MR.
Comment #31
chaitanyadessai commentedPlease review MR from #24.
Comment #32
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #33
akhil babuTried to reproduce this issue, but couldn't. These are the things that I've tried so far in fresh Drupal 10.3 instance (standard profile)
third_party_settings.shortcut.module_linkvalue inclaro.settingstoFALSEthird_party_settings.shortcut.module_linkshould have changed toTRUEas per the hook_install code in the module. But the value after config sync was stillFALSE-------------------------
third_party_settings.shortcut.module_linkvalue inclaro.settingstoFALSEthird_party_settings.shortcut.module_linkshould have changed toTRUEas per the hook_themes_installed code in the module. But the value after config sync was stillFALSESo, unable to create a test for this as @jibran mentioned in #6. Adding 'Needs steps to reproduce' tag to proceed further.
Comment #34
akhil babuAlso, adding
!\Drupal::isConfigSyncing()condition inshortcut_themes_installedbreaks the standard profile recipe test. \Drupal::isConfigSyncing() is true while applying the recipe. Sothird_party_settings.shortcut.module_linkvalue inclaro.settingsis never set to true. As a result, the shortcut module is installed, but the shortcut icon is not added next to titles when the recipe is applied.Comment #36
quietone commentedThe Shortcut Module was approved for removal in #3476880: [Policy] Move Shortcut module to contrib.
This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
The deprecation work is in #3569117: [meta] Tasks to deprecate the Shortcut module and the removal work in #3569121: [meta] Tasks to remove the Shortcut module.
Shortcut will be moved to a contributed project before Drupal 12.0.0 is released.
Comment #37
smustgrave commented