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
In #2336597: Convert path aliases to full featured entities, we convert path aliases to entities. Once that issue lands, we should update the migrations in the Path module to use entity:path_alias
as the destination plugin instead of the custom url_alias
.
Proposed resolution
Update migrations/d6_url_alias.yml
and migrations/d7_url_alias.yml
in the Path module. Change the destination
key as noted and make any related changes that this requires.
Remaining tasks
- Review the patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff-32.txt | 1.37 KB | amateescu |
#32 | 3009014-32.patch | 22.23 KB | amateescu |
#27 | interdiff.3009014.24-27.txt | 2.57 KB | mikelutz |
#27 | 3009014-27.drupal.Convert-path-alias-migrations-to-create-entities.patch | 22.28 KB | mikelutz |
#18 | interdiff.3009014.14-18.txt | 12.76 KB | mikelutz |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis needs to be done before #2233595: Deprecate the custom path alias storage, so here's a patch that a proper destination for path_alias entities.
The combined patch includes the combined one from #2336597-88: Convert path aliases to full featured entities.
Comment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRerolled the combined patch, the "for review" one is the same.
Comment #4
heddnNeeds CR and link to it in this trigger error.
Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThere we go :)
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis tag might be helpful some day.
Comment #8
heddnMaybe this is jumping the gun a little, but I think we're good to go here now.
Comment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks @heddn! Let's re-RTBC it when #2336597: Convert path aliases to full featured entities gets in. :)
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRerolled on top of the latest patch from #2336597: Convert path aliases to full featured entities and updated the deprecation messages.
Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #13
jibran#2336597: Convert path aliases to full featured entities is in.
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch here hasn't changed since #5, so putting it straight to RTBC per #8 :)
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedThe destination plugin is only modifying values on the row so shouldn't we be doing that in a process plugin?
Comment #16
catchComment #17
heddnAgreed, no need for a destination. Let's do this in a process plugin. Doing it in a process plugin is the right thing, but it does lead to BC considerations. Let's see what it looks like first though as a process plugin, then we can work out a BC solution.
Comment #18
mikelutzThis may still have failures and need some cleanup, but I wanted testbot to chew on it while I run to a meeting.
Comment #19
mikelutzPasses tests and no coder issues, I think this is actually pretty close.
This should preserve BC as much as possible.
If you are using the old migration with the new codebase, we've converted the existing UrlAlias destination to write to the new entity storage, but deprecated it.
Running the new migrations on the new codebase uses the new process plugins and the standard entity destination.
I think we may need an explicit test of the legacy destination. I think we lost coverage there when we changed the destination in the migrations.
Setting to NW for that, I'll try to write one if I have time.
Comment #20
mikelutzLegacy tests and BC fixes for the deprecated destination. Okay, I'm happy with this now.
Comment #21
mikelutzActually, let's go this route, it's cleaner.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedHaven't read the whole patch but these caught my attention.
Since this is not the same as the php null coalesce I don't think it should use the same name. It would be better to avoid confusion. There is an existing process plugin that works with an array, array_build, using that as precedent this one could be array_null_coalesce.
The messages thrown in existing process plugin use 'should'. So this could be "The input value should be an array." as is used in the extract plugin. And with the plugin id it would be ,'The input value to the null_coalesce plugin should be an array.'. Or I am being too pedantic.
Why not use ?? here?
Needs more tests. Specifically, with inputs other than NULL.
Use the null_coalesce has a default configuration value instead of another plugin.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedAnd set to NW
Comment #24
mikelutzComment #25
quietone CreditAttribution: quietone as a volunteer commentedThanks for the fixes and explanations, mikelutz. I still disagree with the name but I am thinking about it.
Changed the title to emphasize the change in destination and a few more items.
Should Plugin be capitalized? I am not sure.
This documentation needs usage examples. I'm ok with that being done in a followup.
Looks like this need to check that $value is an array?
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedAdding migrate tag
Comment #27
mikelutzI added an example, fixed the doc, and added some additional error checking.
I wasn't as worried about documenting the Path specific plugin as it's pretty much for this specific migration, not a general use plugin. At some point in the drupal 9 cycle, I think I'd like to refactor and these migrations around the new migration lookup service, but that's not on my radar right now.
Comment #28
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis looks great to me! Thanks a lot @mikelutz and @quietone for working on it :)
Comment #29
jibranThis patch is only for 8.8
Comment #30
jibranOnce I wrote the following
null_coalescing
plugin for a client project.It didn't have a default value support which we do have in this patch and it is a good thinking. Do we need to update https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/li... after the issue is committed?
Comment #31
mikelutzI just had a conversation with one of the Release Managers a few days ago, and was told not to use ?? or scalar typehints anyway, even in new files that will not be backported to 8.7. I'm just doing what I was told. :-)
Comment #32
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't think we should get hung up on that new PHP syntax in this issue, because it's really important for the plan in #3007661: Modernize the path alias system.
Comment #34
larowlanThis looks good to me, but is not my area of expertise, so I'd be happy to see an explicit +1 from a migrate subsystem maintainer
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedYes this looks fine to me. I still have a niggle about the process plugin name, null_coalesce, because it operate on an array. But I don't think the process plugins that share a name with a php function do exactly the same as the php function. In that sense we are not breaking any precedent. Any as heddn says, naming things is hard. Let's get this in.
Found one minor thing that I hope can be fixed on commit. I can't think of a core migration that has a blank line.
delete blank line.
+1 for RTBC
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedOh, and jibran is correct in #30 we need to update the list of process plugins, https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/li....
Comment #38
larowlanCommitted 445132d and pushed to 9.0.x. Thanks!
c/p to 8.8 and 8.9
Can we have those change record updates and followups as per the tags, as well as the update to the process plugin list as per #30
thanks!
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedI'll update the process plugin list now as per #30.
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedUpdated the CR (https://www.drupal.org/node/3013865) and opened the followup to deprecate the
d6_url_alias_language
process plugin: #3087332: Deprecate the 'd6_url_alias_language' migration process pluginComment #42
Wim LeersFollow-up created to refine the migrations now that they have an entity as a destination, and entities can now optionally be validate during migration: #3098985: Follow-up for #3009014: ensure migrations with PathAlias destinations run *after* locatable content entity type destinations.
Comment #43
danflanagan8I know this issue is long closed, but I agree with @quietone that null_coalesce is not a good name for the process plugin introduced here. In addition to being different from the php operator, it's just a cryptic name. I look at the name and I have no idea what it does.
Also it's relatively difficult to spell!
I, like @jibran, have written my own version of this plugin before and I call it first_things_first. That's fun and catchy. A name like first_value or first_not_null would be a lot clearer and easier to spell.
I think null_coalesce needs to get the same treatment that iterator got.
https://www.drupal.org/node/2880427
P.S It's a great addition to core nonetheless. Thanks for the work on this one. I wish there had been a change record that called this addition out on its own.