That all sounds good to me. I also set 'regex' as a schema type so we don't have to add the constraint anywhere we might need to use it in the future.
As a side note, it seems weird to me that 'exclude paths' can't be empty/null, but I checked the code, and fast404 just doesn't do anything unless you exclude *something*. Seems like that could be adjusted, but out of scope here.
All feedback addressed. back to NR.
NW for Phenaproxima's comments.
My suggestion was for a custom solution for your needs, but this was not what the stubbing system was designed for. Also MigrateSkipRowException is going away soon (hopefully), so we don't want to add anymore usage of it in core.
mikelutz → made their first commit to this issue’s fork.
The check against uid 1 is appropriate here, as you cannot run the upgrade path if there are any entities in the system beyond the default ones that exist on install. You can't run the upgrade path as any other user, as your user could be overwritten with a migrated user that didn't have administrator privs, or that wasn't even your account. If migrate_drupal were staying in core in d12, we would have to do some work here to ensure that we were both UID 1 and that UID 1 had an administrator account somehow, which would be difficult as we also couldn't guarantee an administrator role we create wouldn't be overwritten on migration either. Thankfully, we don't have to worry about it since this is all going away, but the use case stated in the original issue summary is invalid.
Since I'm an external worker on the current project I'm working on, I have received an user with the role "Administrator" but obviously it doesn't have the ID 1. So the fact is that I can't execute the upgrades although my user is an admin user.
For this to be true, the system would have to have an additional user created, which means you can't run the upgrades anyway, so the issue is moot.
Talked with @catch, we are going to leave these alone as they will be gone before d12 anyway.
I think we can close this. Migrate Drupal UI is a special case where we really do need to check against uid 1. D7 sites being migrated from effectively have a super user access policy, so it doesn't make sense to have it disabled in d11 in the context of migrate.
With migrate Drupal slated for deprecation and removal by d12, this should be fine to leave.
I need to have a conversation with the fms on this to see what we should do exactly. Yes, the new access policy means we shouldn't make uid 1 special anymore, but migrate is a special case. There cannot be any other users in the system prior to a UI upgrade, so the check is actually reasonable in this case, and with migrate_drupal_ui being deprecated and removed from core in the 11.x cycle, the issue may be moot anyway.
Beyond the nonsense of trying to run the UI migrations without a clean database with no entities, we at least know that coming from d7, uid 1 will be an admin, as uid 1 was special in the database we are migrating from. We can't guarantee this on any other users, so the uid 1 check is appropriate in this situation regardless of the new access policy stuff.
Closing this, I thought there were a couple issues that needed this, but there was only on, so I'll cherry pick this into that issue.
Opened a new MR against 11.x, and added a constraint to validate whether a string is valid regex.
mikelutz → changed the visibility of the branch 11.x to hidden.
mikelutz → changed the visibility of the branch 3437608-add-validation-constraints to hidden.
mikelutz → made their first commit to this issue’s fork.
mikelutz → created an issue.
The test failure is on the display_extenders section. It's a sequence of enabled display_extender plugins, but it's inserted directly from the form values of a mulit-checkbox element. So enabled plugins look like "plugin_id: plugin_id" and disabled plugins look like "plugin_id: '0'". As it is now, validation would mean the value is either an existing display_extender plugin OR the string "0". So the question is do we write that, maybe as an extra option to PluginExists, or a one-off for this situation for views that can be used with the Choice constraint? Or do we start array_filtering the values from the form before storing them, and add an update hook to clean up existing configs first?
mikelutz → made their first commit to this issue’s fork.
mikelutz → made their first commit to this issue’s fork.
Ugh, I missed those, thanks. Feedback addressed. Back to rtbc
This should be ready for review now.
Fixed
Thanks! Looks good now, RTBC pending passing tests.
Looks like all feedback has been addressed here, The removal looks good to me as well.
Looking further, we didn't have the string typehint before we reordered the arguments, so I suppose it's out of scope to add it here. The rest of this looks good.
I have just one tiny nit. Feel free to tell me I'm wrong and I'll let it go and RTBC.
Looks good, still green. Back to RTBC
What about https://git.drupalcode.org/issue/drupal-3442766/-/blob/3442766-remove-de... ? I think that also needs to be removed now?
The changes look good, Thanks!
Looks good, Thanks!
Reviewed the changes, and they look good. Tests pass. I grepped through the directories for additional deprecations and found only appropriate remaining useages of the string 'deprecated' in the Test library. RTBC
Should we add back the legacy state test and set a faux deprecation via reflection to test the infrastructure?
mikelutz → made their first commit to this issue’s fork.
Looking at 📌 Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service Fixed Every other bit around deprecating _drupal_flush_css_js was set to be deprecated in 11.x. I'm guessing because of the odd way of storing the deprecation message here, Our normal way of detecting the bad deprecation message format didn't notice that we forgot it in that string. I think it's good to remove in 11.x
At https://git.drupalcode.org/project/drupal/-/commit/166f3a39e466c2870dcb6... we removed a check and deprecation, but we should have replaced the deprecation with an exception. We removed the deprecation check from the test which left no assertions, but it should have been turned into an exception check.
mikelutz → made their first commit to this issue’s fork.
This is because gin wraps an extra div around tables in its table.html.twig template. There's a few solutions here, but we might need to provide out own table template for stability across admin themes. My immediate workaround is to put the following hook in a custom module in my project:
/**
* Implements hook_webform_analysis_component().
*/
function my_module_preprocess_webform_analysis_component(&$variables) {
$variables['data']['#attributes']['class'][] = 'field-multiple-table';
}
For whatever reason, gin's hook_theme_suggestions switches to a simple table template with no wrapper if the 'field-multiple-table' class is in the render array, so the above code surprisingly works just fine in a pinch.
benjifisher → credited mikelutz → .
This could be done in a contrib module, or with custom code, or even a database query. That combined with the fact that the migrate_drupal modules are about to be deprecated, I'm going to mark this as won't fix.
mikelutz → made their first commit to this issue’s fork.
I think 5 years was enough time for others to provide their thoughts. Layout Builder has matured in core, and I don't see us making these types of deep architectural changes at this point.
Ah, because the commit check only checks the changed files, but another abstract class, MigrateSqlSourceTestBase extends MigrateSourceTestBase, and throws the same error now when all files are checked, and the names were similar enough that I didn't notice CI was complaining about a completely different file.. Okay, I added both to the baseline. Hopefully, this one works.
The @phpstan-ignore-next-line (on the line before the method declaration, not the @dataprovider line worked when I ran the commit check script, but the CI doesn't like it, and I haven't gotten back to figuring out why yet.
mikelutz → created an issue.
mikelutz → created an issue.
I don't recall making this issue, and there have been some changes with the addition of the node_complete migration system that may make this trickier, but I'm curious what breaks If we just do what 5-years-ago-me said we should do.
Sorry for the laxy PR with no tests, but I need the patch for a project right now. Will try to circle back around when I get a chance with tests.
mikelutz → made their first commit to this issue’s fork.
As a Drupal community member and maintainer of the core migration system, I fully endorse Allan Chappell for this position. I have worked alongside Allan for almost four years now, and have seen his commitment to maintaining the contributed ecosystem on Drupal.org. He has 'rescued' dozens of abandoned modules, helping to maintain their compatibility with current Core versions. At Four Kitchens, Allan has taken a leading role in encouraging and mentoring other engineers in contributing to the module ecosystem and has vast experience in both contributing to modules and maintaining them. He would be an excellent addition to a team tasked with ensuring that future abandoned modules are updated.
benjifisher → credited mikelutz → .
Lol. We could, except this project has never had a stable release, so there's never been anything to cover. We went from 7.x-dev to 8.x-1-rc2, and we are back at alpha now. 😂😂 I never got the 1.x branch to a point I was comfortable to call stable. Hopefully soon, with all the recent activity.
No longer postponed, as 📌 Remove usages of MigrateSkipProcessException from core process plugins Fixed has been merged
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
I would argue it is exactly what exceptions are made for: to break out from the ordinary code flow.
But we don't want to break out of the normal code flow here. Skipping rows and finalizing pipelines doesn't happen because of an exceptional condition, bad data, or an error. These are operations that are done by process functions in their normal workflow, and expected and supported behaviors. They are operations on an object, and we have the object available, so it should be able to know its status.
SubProcess shows the need for manually propagate instead of the exception automatically doing so.
I'm not sure precisely what you mean here. I'll assume that you are referring to the fact that the MR has to set the row to skip when it encounters a condition where it wants the row to skip. This is first off only being done to preserve buggy behavior that was partially exposed by this issue. 🐛 When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row RTBC is attempting to fix that bug. It's a prime example of the issues caused by using exceptions to signal things in the normal flow of execution. Some called function deep inside of subprocess throws a Skip Row exception, and subprocess didn't think to handle it, and it bubbles up and breaks the whole outer row. This makes it more difficult to code those kind of bugs.
Look at the number of checks for skipping before and after. What use cases are made possible by adding this complexity?
This code is in a transition state, but the ultimate goal here is to remove the usage of MigrateSkipRowException and MigrateException (the latter of which does the same thing as MigrateSkipRowException, but allows you to set an arbitrary row status while bypassing the row) in process plugins, and replace them with a single set of api calls on the row object itself. This makes logical sense to me, as the row object should know and report its status. As far as the number of skip checks, with the exception of sub_process, these are 1:1 with the existing try-catch blocks which are going away. and 🐛 When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row RTBC currently adds a try-catch block there too, which will be replaced by a skip check either here or there depending on which issue is committed first. The endgame code will be able to replace multiple catch blocks with a single API check, so we are reducing complexity.
What problems are being solved here?
See above comments on reducing complexity and preventing unintended bugs
Also, I am not sure why is it called "getSkip". Let's look at events: the method is called isPropagationStopped so the analogue would be isProcessingSkipped.
Alwasy happy to debate naming conventions, the PR is a WIP, but initially it as a simple getter and was named as such, but something like isRowSkipped would work too. I'm going to see what it all looks like once the migrateException stuff is included and see what makes the most sense.
mikelutz → changed the visibility of the branch 2329253-allow-the-changeditem to hidden.
Assigning to me for more tests and features, and requested changes.
Hmm, I'm not sure this will be something we can directly support in the module if the pelcro JS is firing a subscription create event prior to payment being completed. We would have to see if there is any way for us to tell inside that event that the payment has not completed, and ensure that that event or another one is fired after payment is actually completed. Based on the documentation here: https://docs.pelcro.com/v1.67/docs/js-sdk#events. I'm not sure that either of those is the case. IF pelcro is creating the subscription prior to payment being completed (or at least firing the event) we may have trouble with this.
It doesn't appear to need a rebase. That issue was merged on Jan 12, and I picked this back up and rebased on Jan 13. It's still NW because the refactor needs more tests, and I'm playing with adding the ability to set the row map status on skip, which would let us deprecate using MigrateException inside a process plugin to skip a row and mark it failed. Hoping to get to it this week.
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
Dang. I had a whole thing typed up here, but I must not have hit submit. Here goes trying to remember what I was going to post...
This is a bug, NW for tests, but I'm not certain if this is the right approach here.
So the core EntityConfigBase expects the entity id key (generally id for configs) to contain the whole id. for a field storage entity, that would look like "node.field_text' we have 12 plugins that extend EntityConfigBase for various reasons. The three addressed in the MR are the only ones affected by this bug, as they override the getIds() method to change the destination plugin's ids and map table storage such that the individual parts of the config id are separate properties in the migration (i.e entity_type: node, field_name: field_text)
And as mentioned above, and fixed in this patch is the issue that the base getEntity method (from \Drupal\migrate\Plugin\migrate\destination\Entity) can't handle that, it tries to load an entity using only the first element of the ids array.
So there are two interesting things to note here. The first is EntityBaseFieldOverride which needs to do a similar thing but does it in a different way. Instead of overriding getIds, it overrides getEntityId instead. I got a bit into the weeds with trying to understand it, but it seems like this works for base_field_override, and might have worked for field instances because of the FieldConfigBase id() method, but it won't work in general.
The main thing that give me pause is this bit of EntityConfigBase's import method:
if (count($ids) > 1) {
// Ids is keyed by the key name so grab the keys.
$id_keys = array_keys($ids);
if (!$row->getDestinationProperty($id_key)) {
// Set the ID into the destination in for form "val1.val2.val3".
$row->setDestinationProperty($id_key, $this->generateId($row, $id_keys));
}
}
So, EntityConfigBase is smart enough to know that if a subclass has overridden the getIds() method, then it needs to implode the non langcode ids with a '.' separator and load it into the 'id' property so the config can be created with the right id, but it doesn't do the same with the old_destination_id_values before calling getEntity(). So my gut says we should fix this in EntityConfigBase, either by manuipulating old_destination_id_values before calling getEntity(), or more likely by overriding getEntity and putting the logic there (That way subclasses could still override getEntity with the original old_destination_id_values if they wanted to.
NW to refactor this into EntityConfigBase for any subclass that overrides getIds() and for tests.
Made some adjustments that broke some tests, and I'm still debating if they are worth keeping at all. Currently, when SourcePluginBase runs all the prepare_row hooks, if a hook return false, the remaining hooks still run, and the row is skipped, while if a hook throws a skip_row exception, the remaining hooks are skipped and we go straight to the catch block after the hook invocations. The original version of this patch left it so that calling $row->skip in a prepare row hook still let all the remaining hooks run before deciding to skip the row. While this difference in behavior from the exception is fine (We don't rely on the behavior in core, and it's okay for the replacement of a deprecated api to work slightly differently) for performance reasons, it seems like it would be nice to stop hook processing once a skip is flagged. The latest commits do that, but broke a few tests that will be a little tricky to rework. I think ultimately we should keep the efficient behavior and eventually deprecate the ability for prepare_row hooks to return FALSE as a signal to skip a row, requiring them to use $row->skip instead, but I don't have time to fix all the tests today.
✨ Allow process plugins to stop further processing on a pipeline RTBC has now been merged and this issue is no longer postponed.
MR updated and passing. Ready for review.
Change record updated.
Everybody saw it, Longwave said I could change the interface!
Seriously, that is my preference, I've just gotten pushback in the past when following the BC promise to the letter when there is an alternative that goes above and beyond, hence the new interface. It does seem like lately we moving away from going above and beyond the BC promise when doing so involves adding extra unnecessary code and interfaces, so I'm all for it.
Still needs work for #34
This is a bug, The explode plugin should accept any value that PHP's 'explode' function will accept, which is any string not equal to the empty string ("")
Cross post with cilefen. I didn't mean to undo his status change, though I'm still unsure how 🐛 Regression from #2521800: using machine name element for ListStringItem breaks with existing data Fixed would affect the actual settings form.
The issue with the fields is related to 🐛 Regression from #2521800: using machine name element for ListStringItem breaks with existing data Fixed But I do not immediately see why that would prevent saving the settings form itself. That issue is specifically related to list fields on entities, it shouldn't affect a config form, and I see nothing in core behind the scenes that would trigger any validation around the fields when saving just the settings form.
Un-checking the "Contact settings" prevents a new user from trying to register
while the test system is online. Does anyone happen to know the area in the
Drupal database that the "Contact settings" can be set, which is likely a simple
boolean value?
Contact settings would be stored in the 'user_default_enabled' key in the contact.settings config file, though new user registration would be the 'register' key in the user.settings config file, where a value of 'admin_only' would prevent registration. Both are simple configurations that could be exported at /admin/config/development/configuration/single/export and copied into /admin/config/development/configuration/single/import with a modification and saved
CR looks good, new test looks good. The "fix" is very straightforward, most of the discussion on this issue is around whether to define this as a bug, and I'm confident that it is. The CR provides options for those that wish to replicate the buggy behavior. I'm marking this RTBC.
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
This needs to have the merge request updated to 11.x, or a new MR created against 11.x
My only concern with the approach is that the prepare row method and hooks can also write messages to the table, and if they aren't explicitly skipping the row by returning false then any messages created by the row preparing will start accumulating. Maybe we should check whether any messages exist for the row before we restore instead of just checking if the row will be skipped due to track changes/highwater/needs update etc.
I was also a little worried about performance, I ran some tests on a 40k row migration without the map table joined. The migration was completed, so no rows were actually processed. I inserted a message for each row and ran a few times with and without the patch. I was seeing ~15seconds or 6% additional runtime on average with the extra database trips per row. It's not insignificant, but it could be worse, it extrapolated to an extra 6ish minutes on a million row run that would have taken an hour and a half already, so maybe it's worth providing a way to opt out? I'd approve the MR without an opt-out, but I'll let others weigh in.
benjifisher → credited mikelutz → .