Closing this issue, as the problem seems to be with some incompatible custom templates on our end. The feature seems to work fine in stock mode.
Scary to se a 2020 patch still not commited to drupal/blockquote>
It's not committed because it doesn't fix any bug with Drupal. It only hides an error due to a corrupted database. There are plenty of instructions in this thread on how to fix the underlying problem with your site.
I'm inclined to do it, since we can generate the baseline and don't have to put it together manually, but it's ultimately up to the committers and RMs. Contrib and custom code will be far more likely to be alerted through phpstan, as contrib and custom test coverage is likely to be spotty at best.
@longwave, Do we need to add the @deprecated annotation for DrupalSqlBase, or should that just be covered with the deprecation of the whole module eventually?
mikelutz → changed the visibility of the branch 3507572-deprecate-migrate-source to hidden.
I like that, it’s pretty slick and much fewer lines than I was expecting to have to deal with. I’m inclined to try to get this merged in early in the 10.3 cycle so that contrib has time to deal with the fallout. It leaves the ‘hard’ problem of identifying each of the source plugins the legacy tests for the actual removal when the 12.x branch opens, but at least we don’t have to do that twice, once for @legacy annotations, and again for removal. And when we get around to doing it, we could just remove the suppressions to get a list of all affected tests easily enough.
benjifisher → credited mikelutz → .
What about callback plugin provided by the core, don't we need to implement logic to allow skipping the process inside callable?
We do not. The primary purpose of the callback function is to call built-in PHP string transformation methods on the pipeline. Any attempt to provide a mechanism to allow methods called from callable to access the pipeline stopping mechanism here would only work if the called methods were aware of it (i.e., custom methods). Since we are talking custom code at this point, the "workaround" is to implement your logic in a process plugin instead of a method to be used with a callable.
There might have been an argument to be made in 📌 Deprecate MigrateSkipProcessException Fixed that with the deprecation of the skip process exception, there is no longer a method for callable invoked methods to skip the process, but given the primary purpose of callable mentioned above along with the "workaround" for custom code, This is a change we are making intentionally.
What should happen here is to add a note about this to the change record at https://www.drupal.org/node/3414511 → , stating skipping the remaining process from inside a callable callback is no longer supported as of Drupal 12
I added a link to the original CR for isPipelineStopped with the before and after.
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
mikelutz → created an issue.
This is now unblocked
There is nothing to review here, the MR has no commits.
I don't think the test changes are any good. Neither undoing the fix nor removing d7_menu from the test's list of migrations will make the test fail. So I'm leaving the Needs Tests tag in place. But I'm marking this as Needs Review in hope that a subsystem maintainer will advise on what might be wrong or an alternate test strategy.
You can't make a test for this based on looking at the system after the migrations have run, because the migration and the system do not care what order they are run. If the block runs first, it's saved with an invalid plugin for a hot minute, and then when the menu migration runs, the plugin is valid and everything is fine. So you can't do a test based on the final system state; the only way to test is to inject a mock logger or check the logs to make sure the warning is never logged.
I'm not opposed to documenting that limitation and moving on without it. Alternatively, we could just load the module files prior to installation, same as we are now doing with the classloader. I didn't code up any solutions because I wanted some comments on whether we should worry about it at all. I mainly wanted to surface the issue, as we did the work on the classloader to ensure OOP constants and enums in a module's src folder were loaded and accessible during config install.
Also, to clarify for anyone reading this in the future, constants in .module files of active modules do work. The issue here is specifically happening when installing a module whose install config references a constant defined in the .module file. It's only during module install where a module's config files are loaded without the .module file being loaded.
I suppose because CONFIG_ENUM_TEST_VALUE is a global const defined in config_enum_test.module, but the module file is not preloaded.
That is the test I added in #87 to show the issue regarding loading the module file. It hasn't been addressed yet in the MR
benjifisher → credited mikelutz → .
Okay, I did some further review on this, and it's looking good. I did some git magic to compare this MR against what 11.x would look like with the annotation conversion reverted to make sure all the expected source plugins were reverted and they were.
RTBC from me.
I posted a MR that blindly follows instructions in the issue summary, as I am still learning XB and have no context. Just trying to get my feet wet with contributing to XB.
mikelutz → made their first commit to this issue’s fork.
Marking this as critical and an alpha blocker, as this is undoing changes from earlier in the 11.2 development cycle that we absolutely do not want to see in a release.
I did a quick review of the new MR, and at a glance, everything looks good. I want to spend a little more time and/or have a couple of other sets of eyes on it before I RTBC, but I do think this is likely good to go.
The test seems fine. We had no test coverage around NoSourcePluginDecorator before, so this is a good opportunity to add some.
Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?
I would say this exposes a lack of test coverage, but since the class essentially exists for the one line we are changing here, it's best to add the coverage here. I would say the majority of the seven questions are no's in the new guidelines, which means the tests are okay to add
Is the fix is easy to verify by manual testing?
no
Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc.
no
Is the fix achieved without adding new, untested, code paths?
no
Is an explicit 'regression' test needed?
no
Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?
yes
Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?
yes/no
If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?
yes
We can use this issue.
hongpong → credited mikelutz → .
I am still of the opinion that we shouldn't have converted any of d6 and d7 source plugins to attributes at all. Leave source_module in the annotation, leave all the sources as annotations, and depreciate them. They all have to go before D12 anyway. There was never any reason to deal with source module in attributes at all. We haven't done a release with the conversion yet, we could still execute it this way and not worry about it.
Can we manually clear that apcu cache in migrate drupal's hook_install? along with the source plugin discovery cache?
I think we can close this. We ended up reverting the patch that surfaced this issue anyways, and I don't know that it needs an explicit test anymore.
Merged, thanks!
mikelutz → made their first commit to this issue’s fork.
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
benjifisher → credited mikelutz → .
Feedback addressed, and I gave it a once over myself, this looks good to go.
I've updated the documentation with @quietone's suggestions. Setting back to review. We still need to consider if we want followups for the plugin bases that use different merge strategies or if they are fine as they are.
Fixed/tweaked a few docs while I was in there, setting back to NR
NW to bikeshed method names. :-)
smustgrave → credited mikelutz → .
Since the process pipeline is an array, maybe it would be more suggestive to use brackets instead of parentheses. For example, using a line from one of the tests in the MR:
"d7_field:type:process_field(1): Can't migrate field 'field_event' with 'todate' settings. Enable the datetime_range module. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#datetime"
What do you think of this instead?
"d7_field:type:process_field[1]: Can't migrate field 'field_event' with 'todate' settings. Enable the datetime_range module. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#datetime"
It's a shame how difficult it would be, given how we build up that message, but
"d7_field:type[3]:process_field: Can't migrate field 'field_event' with 'todate' settings. Enable the datetime_range module. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#datetime"
Would be much better. It's not the third call to a process_field plugin, it's the third process plugin called for the type destination property.
But to do it you would have to add another property to migrate exception, and bubble the value up, set it in the right places, deal with the times a migrate exception is thrown when an index makes no sense, ect.
ooh, I really like that. I had given thought to something more general in the batch system to prevent duplicate batches from being set, but I was thinking about thinks like having batch_set hash the definition array and preventing duplicates that way, but it would be a behavior change, and I can't be certain that someone doesn't have. a legitimate reason to duplicate a batch somewhere for some reason, so I kept the focus on the rebuild.
This is a simple and elegant helper method in the helper class for code to ensure it doesn't add a duplicate batch if it doesn't want to.
Switched to using a static variable, and added a test.
mikelutz → made their first commit to this issue’s fork.
Closing MR and hiding files as we decided on a documentation only solution for core. Work on a UI should be done in drush or migrate_tools.
mikelutz → changed the visibility of the branch 2713327-provide-a-way to hidden.
Not a huge deal, but links meant to be external-only are getting and can’t be saved.
This should probably be a new issue, I'd like to explore this a bit. We may want to add a 'allow empty' option or something, but I'd need to see some tests (and go back and look at D7) to see how we could get ourselves into a situation with a blank external only uri and what happens in the migration.
The changes to the migration form make sense. The tests in Drupal\Tests\migrate_drupal_ui\FunctionalJavascript\SettingsTest all pass, so that looks good to me as a migration maintainer.
Opened 📌 Harden installer translation file download against potential infinite redirects Active as a follow-up
mikelutz → created an issue.
smustgrave → credited mikelutz → .
mikelutz → created an issue.
mikelutz → created an issue.
benjifisher → credited mikelutz → .
@joachim is correct here. The migration process definition is everything under the `process` key in the migration yaml, which is what the instructions are referring to. The process definition consists of multiple mappings of destination keys to process pipelines, and each pipeline consists of multiple process plugins.
In this context, the statement "... the migration process definition must include mappings for the entity ID and the entity language field." is correct. Replacing the word 'definition' with 'pipeline' would not make sense, as the migration has several process pipelines, one for each process definition mapping.
To be fair though, while this is the technical nitpicky definition, we are fairly inconsistent in the way we use these terms in internal discussions. THe maintainers often rely on context to determine exactly what is being talked about, and I wouldn't be surprised to dig through and find inconsistencies in the documentation too.
Anyway, I believe this issue is good to go.
I banged my head on this for an hour this morning, just to figure out it was Drupal killing my brackets and not a problem with my regex...
The answer is to add the 'allow_square_brackets' option to the query
`\Drupal::database()->select('table', 't', ['allow_square_brackets' => TRUE]);`
Unfortunately, my regex was `'[0-9]{2,}'`, and I don't see a way to preserve squiggly brackets as those are used as table placeholders by the query builder. I'll get around it with `'[0-9][0-9]+` for today, but it's good to realize I can't use squiggly brackets in regex.
+1 to back to RTBC
After all the discussions here, and in slack I think it really is this simple. I've updated the change record here → . I really think the best thing is to get this into 11.2 now, as early as possible, just in case there are any unexpected patterns in contrib that this might negatively affect. This will give us as much time as possible to identify and adjust.
Looks good. I concur with @longwave on the fix, and I'm happy with the comments.
Just need to tweak the comment above those sections in the installers referencing modules that need container rebuilds "immediately after install". We also will need to update the CR here → at some point after this is merged.
I think you meant to postpone this on 📌 Install modules with container_rebuild_true set by themselves Active , no?
I think this will increase the index twice if we install two $container_rebuild_required modules in a row. This would probably work, but we would call module install with an empty array.
Drupal::service() calls in hook_install() is a different problem I had not even really thought of, but there is no way for modules to know that another module is going to do that, so container_rebuild_required won't help. However that also seems pretty fragile anyway. I guess we could try to detect when we've just installed a module that another module we're about to install depends on, and rebuild the container then?
Is this an issue though? From a brief reading of the issues and code it seems like we rebuild the container for the batch prior to running any of the install hooks for the batch anyway. We also rebuild the container before the config installer is used, so I'm not even sure that config_selector would be an issue, as we would be using the updated config installer for the batch containing config_selector too. Seems like what would be different would be on hook install you would have items in the container that wouldn't have been there previously, i.e. some service might be decorated from a module dependent on this module that wouldn't have otherwise been decorated when this module is being installed.
So if module a depends on module b, and module a decorates some service, currently it knows that when module b is installed it is using the original service and not the decorated service. With this change it can't be sure of that, and the flag won't help. But I can't think of a scenario or example that would make that an issue? Possible though.
I'll trust that the change is correct as far as the fedex credential labeling. I like the new wording.
Merged and fixed.
mikelutz → made their first commit to this issue’s fork.
mikelutz → made their first commit to this issue’s fork.
mikelutz → made their first commit to this issue’s fork.
mikelutz → made their first commit to this issue’s fork.
mikelutz → made their first commit to this issue’s fork.
mikelutz → made their first commit to this issue’s fork.
Fixed, Thanks!
The StringTranslationTrait is an essential component to allow for translations into other languages. Removing it is not an acceptable fix.
You are 2/3 correct. The StringTranslationTrait is an essential component to allow for translations into other languages. It is not a fix for the original issue here, because the trait has nothing to do with that error. The signature of the packer service was changed between 8.x-1.0-beta3 and 8.x-1.0-rc1. The error came from updating the code without clearing the cache. At some point while modifying the files, OP must have cleared the cache which caused the problem to go away.
However, It is acceptable to remove this. Our packer doesn't directly use string translation, and the default packer that it extends from already uses this trait, so there is no need to use it a second time here.
mikelutz → made their first commit to this issue’s fork.
mikelutz → created an issue.
mikelutz → made their first commit to this issue’s fork.
smustgrave → credited mikelutz → .
gábor hojtsy → credited mikelutz → .
smustgrave → credited mikelutz → .
I don't see that in the latest code for paragraphs module:
> interface ParagraphInterface extends ContentEntityInterface, EntityOwnerInterface, EntityNeedsSaveInterface, EntityPublishedInterface {
I just noticed that the actual methods of EntityOwnerInterface are deprecated on the Paragraph entity, but I don't know the whole story or for how long that's been the case.
https://git.drupalcode.org/project/paragraphs/-/blob/8.x-1.x/src/Entity/...
I'm not sure what exactly we could do here, except carve out an exception in core for paragraphs. Two issues here, first, under the current code, the paragraph owner is the owner of the parent entity. Your issue isn't that the paragraph is owned by anonymous, it's that you haven't given it a parent yet at the time you are trying to validate it. Secondly, it seems that paragraphs has deprecated their use of EntityOwnerInterface, meaning in the near future, even if you did assign the parent during the migration (which is not something you would typically do) It's still going to not provide an owner to validate against though the normal entity api. You would have to know it was a paragraph and that paragraphs validate in the UI through the parent's owner, then go find that (possibly through nested paragraphs) specially for paragraphs, which we won't do in core.
Without paragraphs providing an owner to validate against, there isn't much we can do, since we can't really validate, because we don't know what permissions to validate against for things like text format of a body field.
I think this might have to be paragraph's problem. I don't see how we can support validation in this instance if paragraphs is going to do something funky outside the entity api in it's own validation.
I am open to suggestions though.
This is neither reasonably possible, nor desired. The map table is a record of what we did, not something that reflects the current state of the system. More importantly the meaning of destination ids is completely plugin defined. They have no relationship to entities at all, so there would be no way to connect a deleted entity to a map table entry. The issue summary describes a custom workflow and problem that is best solved in custom code, where the custom code is aware of the specific relationships between migration destination maps and entities in use in that particular site.
I just want to note that "wrap" is just syntactical sugar for
plugin: get
source:
- ~
and unwrap is just syntactical sugar for
plugin: extract
index:
- 0
With maybe some single_value, multiple_value plugins in there if you need them, though if the purpose is to pipe a value into a handle_multiples plugin that can't handle a single value, there will often not be much point, as those plugins don't care if the value is a multiple or not.
(BTW, I'm starting to come around to just documenting the 'hack' with the get plugin above and officially supporting it. I've found enough cases where it's useful)
benjifisher → credited mikelutz → .
hestenet → credited mikelutz → .
hestenet → credited mikelutz → .
mikelutz → changed the visibility of the branch 3463800-allow-batchsize-to to hidden.
mikelutz → created an issue.
mikelutz → created an issue.
mikelutz → created an issue.
mikelutz → created an issue.
For lack of my 7am brain being able to come up with a clever way to do this in bash, running
<?php
$f = fopen("5278.diff", "r");
$regxp = "/https:\/\/([a-z0-9A-Z\.-]+)[^a-z0-9A-Z\.-]/i";
$matches = [];
$results = [];
while (!feof($f)) {
$line = fgets($f);
if (preg_match($regxp, $line, $matches)) {
$results[$matches[1]] = isset($results[$matches[1]]) ? $results[$matches[1]] + 1 : 1;
}
}
array_multisort($results, SORT_DESC);
foreach ($results as $name => $count) {
echo $name . ": " . $count . "\n";
}
on the original MR gives
example.com: 1412
www.w3.org: 747
www.example.com: 136
jsonapi.org: 77
jqueryui.com: 58
jquery.org: 48
www.drupal.org: 40
api.jqueryui.com: 37
drupal.org: 22
www.test.com: 20
wikipedia.org: 18
google.com: 14
docs.jquery.com: 9
www.doctrine-project.org: 8
fonts.fontprovider.com: 8
php.net: 7
www.sqlite.org: 7
en.wikipedia.org: 7
api.drupal.org: 7
www.collegehumor.com: 7
symfony.com: 6
along with ~80 additional domains with < 5 usages
The numbers are a bit off from the actual work between codebase changes and the vendor changes to be undone. Also the xmnls in the svg files should not be changed, https://www.w3.org/2000/svg is an identifier, not a url, and it should not be https. Still probably a good baseline to start with. Having discussed scoping child issues of metas like this several times with @xjm in the recent past, I'm fairly confident we should be doing this by domain for the top domains and then catch the one-offs in a separate issues or issues as it makes sense. These are probably not novice issues (at least not all) as there are many places where this should not be changed at all. Let's start with a child issues for
example.com: 1412
www.w3.org: 747
www.example.com: 136
jsonapi.org: 77
jqueryui.com: 58
jquery.org: 48
www.drupal.org: 40
api.jqueryui.com: 37
drupal.org: 22
www.test.com: 20
<\code>
and go from there. We can also probably standardize on replacing http://example.com with https://www.example.com and http://drupal.org with https://www.drupal.org while we are at it.
That's an option, but let's hold back. Even with reduced scope, this can't go in before 11.1, as we are already in RC for 11.0 and this is unlikely to be backported to the 11.0 series as it's a new feature and not a bug fix. If we can't make progress on the parent issue in the next couple of months we can revisit the reduced scope here to try to get it into 11.1, but in general, I like the scope and functionality, the timing is just bad with the other API changes we are working on.
AFAICT you can fall back version_compare() unless BOTH left & right are 11.x and 11.0.x (though not sure what order they come in, might have to check both orders).
Plus, doing this, would just cause the problem to come around again when we open 11.1.x in a few weeks.