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.
Not the decision maker but just going to re-iterate, I think this is a bad idea. The hacks I see for uninstalled modules seem like just the tip of the iceberg. What about removed modules. removed libraries. uninstalled themed. profiles.... it seems like a loosing battle for little benefit.
I'm not fully seeing the issue here. What do you mean about removed modules, removed libraries, etc? If you uninstall/remove code providing a constant/enum used in your active configurations, you will have problems, as you should. The 'hack' here only adds a module to the classloader as it's config is read in preparation to install it. This seems appropriate to me, as the config you are installing should be able to use enums defined in the module you are installing. This isn't about loading enums and consts from uninstalled modules, this is to load enums and consts from a module that we are in the process of installing. I really think the inline comments are scarier than what the code is doing, and I made a couple suggestions on the comments. I also added a test that I think shows another issue, config referencing constants that are in the modules .module file. I think we will have to include the .module file before parsing the config as well.
Also, I tried to find a more elegant way to prevent double caching of the autoloading storage, but I really couldn't, so I just added a second instanceof check in the Comparer. I don't like it.
I'm going to move it to a full fledged constraint in Core/Validation to solve the dependency issue. To make it work for both snippets and full pages, I think we can have the constraint just wrap snippets in "" .. "". We could either define whether to do so in an argument to the constraint, or try to autodetect, but I'm going with a constraint argument that defaults to snippet because the schema would want to decide which it allows. The fast_404 html here would want a full page and want to reject a snippet so I don't see autodetecting being practical.
(Ironically, I was resurrecting 📌 Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface Needs review the other day and I realized the we nearly did the same intercomponent dependency in the patch we committed and reverted all those years ago. We were going to call Component\Utility\NestedArray from Component\Plugin and didn't notice)
Okay, I reviewed this pretty thoroughly as it was being built and gave it a final once over. As far as I can tell, we've covered all edge cases, tests look good, documentation looks good. Let's ship it.
Postponing this on 📌 Allow process plugins to flag a row to be skipped Needs review , as there are a number of things in here that don't make sense in the context of the changes we are trying to make to skipping rows, such as doing away with using Exceptions to do so.
Alright, I think I've resurrected this from the ashes. New merge request on 11.x, fixed issues in #123and #132
123.1 Mention the interface instead of the trait in ConfigurablePluginBase.
I mention both, since the base class uses the trait anyway.
123.2 In ConfigurableTrait, document that the $configuration property is also defined \Drupal\Component\Plugin\PluginBase.
done
123.3 Decide on if ConfigurableTraitTest needs adjustments.
It did, though opposite from the previous suggestions of moving all tests to getMockForTrait, as getMockForTrait is getting deprecated in phpunit, so I converted all tests to use the same concrete test class that uses the trait.
123.4 Make sure that the changes in base classes for certain plugin types are mentioned in the release notes.
I added the list to the release notes snippet and the CR
123.5 (optional) Check if setConfiguration() still needs to be overridden in WorkflowTypeBase.
It's an issue of overriding it and keeping tests, or resetting it to the standard and adjusting all the tests to handle the reordered returned array. I also removed use of the trait entirely from ConditionPluginBase for the same reasons, updating it to use mergeDeepArray broke a few tests and will require adjusting the order of items in umami config to pass tests (as they would be exported in a different order, causing a test fail) It's better to explore converting both of those pluginbases to use mergeDeepArray in a followup, if at all, where we can review the test changes and potential BC issues more thoroughly in isolation from the trait and base class work being provided here.
123.6 Rebase the merge request.
done.
132 > implement \Drupal\Component\Plugin\ConfigurableTrait directly
Is that the right wording? Does one 'implement a trait'? I'd have said 'use' or 'import'.
I agree, switched the wording to use `use`.
In addition, I added SelectionPluginBase to the plugin bases that we convert (it was added after the original MR was created here), and removed ConditionPluginBase for reasons described above.
I moved ConfigurableTrait from Component/Plugin to Core/Plugin. In the original patch, we didn't notice that having the trait in Component meant we added a undocumented dependency on Component/Utility to Component/Plugin, and we shouldn't do that. The interface can live in the Component, the trait can live in Core next to the base class (which was already there because that needed to extend Core's PluginBase and not Component's. Despite the fact that the interface lives in Component, it makes more sense to have the implementations live in Core.
I updated the Change Record (and unpublished it, as it was never unpublished after we reverted years ago.
I fixed a few coding standards/phpstan/phpunit10 issues that have changed things over the years.
I think this should be committable now, or close to it hopefully.
cross posting from 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work
I think we need to rethink this, and 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work in light of 📌 [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL Fixed
source_module is a migrate_drupal thing, as this issue points out, but despite the work here, the source_module key is still a thing defined in migrate as part of the source plugin annotations, and 3421014 is moving it over to the attributes defined in migrate. I'm working out the order of operations here, but I think the concept of source_module as a source plugin attribute/annotation key needs to be deprecated in Drupal 11 along with everything else migrate_drupal.
I have not followed the raw details on the annotation to attribute conversion for plugins, and I'm not sure what would break if we did this, but my gut says that here, we should NOT support source_module as a source plugin attribute, nor should we convert the d7 source plugins that we are deprecating in D11 over to attributes. I'm sure there's some core test somewhere that ensures core plugins which support attributes use them, and we may have to find a way around that for the sources we are deprecating, but I feel like it's worth it, and it saves us from having to deprecate source_module as it can just go away with annotations.
I think we need to rethink this, and 📌 Convert MigrateSource plugin discovery to attributes Active in light of 📌 [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL Fixed
source_module is a migrate_drupal thing, as this issue points out, but despite the work here, the source_module key is still a thing defined in migrate as part of the source plugin annotations, and 3421014 is moving it over to the attributes defined in migrate. I'm working out the order of operations here, but I think the concept of source_module as a source plugin attribute/annotation key needs to be deprecated in Drupal 11 along with everything else migrate_drupal.
I have not followed the raw details on the annotation to attribute conversion for plugins, and I'm not sure what would break if we did this, but my gut says that in 3421014, we should NOT support source_module as a source plugin attribute, nor should we convert the d7 source plugins that we are deprecating in D11 over to attributes. I'm sure there's some core test somewhere that ensures core plugins which support attributes use them, and we may have to find a way around that for the sources we are deprecating, but I feel like it's worth it, and it saves us from having to deprecate source_module as it can just go away with annotations.
mikelutz → changed the visibility of the branch 2852463-create-a-trait to hidden.
mikelutz → changed the visibility of the branch 9.2.x to hidden.
mikelutz → changed the visibility of the branch 2852463-configurable-plugin-trait-10.1 to hidden.
mikelutz → changed the visibility of the branch 11.x to hidden.
mikelutz → changed the visibility of the branch 11.x to hidden.