Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#842620: Update manager can't install modules using FTP due broken FileTransferAuthorizeForm removes all usages of the system.authorize config. This issue will remove it from the code base and provide an update function to remove it from active config.
We shouldn't be storing these details in config because they are environment specific and actually as form fields we can rely on a broswer to remember them - also storing any user credential for ssh or ftp even a user name seems like a bad idea.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#39 | interdiff-2715145-35-39.txt | 799 bytes | phenaproxima |
#39 | 2715145-39.patch | 4.6 KB | phenaproxima |
#35 | 2715145-35.patch | 4.49 KB | andypost |
#35 | interdiff.txt | 1.01 KB | andypost |
Comments
Comment #11
longwaveRediscovered this via #2591827: Add YAML linting to core coding standards checks where we detected that system.authorize.yml is invalid.
There is also now a migration that writes config to this file, but the config is never used at runtime.
Comment #12
longwaveComment #13
andypost++ to remove and check 2 usages in contrib (both used to exclude it) so removal without deprecation looks straight
http://grep.xnddx.ru/search?text=system.authorize
I bet it needs upgrade test
Comment #14
andypostFiled CR https://www.drupal.org/node/3206320 and here's a test
Comment #15
andypostFix c/p typo
Comment #16
longwaveThanks for adding the test and CR, the test looks good to me.
The only thing I'm not sure about is whether we can remove the migration directly or if we have to deprecate it first somehow? Marking RTBC to get core committer feedback on this.
Comment #17
renatogWe should change this comment
Remove obsolete system.authorize configuration.
to
Removes obsolete system.authorize configuration.
Because this same standard is used in other comments. E.g.
Just to be consistent with the same way on this file
Comment #18
longwaveI am not sure that is always the case in update hook docblocks, which are displayed to the user during update.php - we are a bit inconsistent:
Comment #19
renatogMakes sense.
Ok, it was just it from my side.
Moving to RTBC
Comment #20
alexpottThe standard as per https://www.drupal.org/node/1354#updateN is to use
Remove obsolete system.authorize configuration.
Comment #21
renatogMakes sense
On the same patch, we're including another document using a different way
Should we use the same standard to be consistent? E.g.:
Comment #22
andypost@RenatoG no, only update function doc blocks are without-s other functions and classes has common rule
Comment #23
renatogOk makes sense.
So +1 to it
Comment #25
benjifisherPlease do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.
Comment #26
alexpott#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.
Comment #28
longwaveHit by #3208791: [random test failure] Random fail in LayoutBuilderDisableInteractionsTest, back to RTBC.
Comment #29
catchCan we just remove this or do we somehow need to deprecate it? Not sure what the implications are for the various migration runners if this file disappears.
Comment #30
longwaveI asked that in #16, I don't know the answer either!
Comment #31
longwave#3039240: Create a way to declare a plugin as deprecated IS suggests we do need to deprecate it, but we can't yet because of that issue
Comment #32
longwaveLet's follow #3022401: Remove useless config action.settings.recursion_limit and keep the migration but make it do nothing, so we don't have to postpone on plugin deprecations.
Comment #33
andypostGood idea, thanks! changed to suggestion from #32
btw in #3039240: Create a way to declare a plugin as deprecated deprecated migrations are allowed to be skipped so better to improve deprecation when it lands for both (actions and migrations)
Comment #34
longwaveDo we need a test similar to MigrateActionConfigsTest to prove that the updated migration doesn't write any config?
Comment #35
andypost@longwave Thank you!
Reverted execution of migration and extra assert added
Comment #36
longwaveGreat work! RTBC if bot agrees.
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedI thought that assertions weren't to have messages, #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages. This could be a comment.
In my experience, there are none and in this case the migration can be deleted. If the migration id does not exist it is simply ignored. For example, drush (migrate run) typing 'drush mim 'migrationwithtypo' returns nothing, which can be annoying. But this also allows migration lookup to be very flexible in the list of migration. See 3143486#7. But to be sure let's get another opinion from a migrate maintainer.
Comment #39
phenaproximaFixed the comment in the test.
I'm a lapsed Migrate maintainer, but from my cursory reading of https://git.drupalcode.org/project/migrate_tools/-/blob/8.x-5.x/src/Comm..., it appears to gracefully handle missing migration IDs. I haven't personally tested that, though,
Comment #40
longwaveThe current patch just converts the migration to do nothing, so the issue of whether or not deleted migration plugins cause any issues is not a problem. The test comment was fixed and the test improved, so back to RTBC,
Comment #41
alexpottCommitted b36543d and pushed to 9.3.x. Thanks!