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.
Closing this, as we just use ConfigurableInterface now.
mikelutz → created an issue.
NW for @quietone's requests. I left a few comments with some thoughts for folks to think about, but none are hard requirements for this MR, they could be done here if they make sense, or they could all be debated and done in a follow-up (or not)
Oops, yes, thank you.
PP on 📌 Add PHP_INT_MAX to integer schema configuration Active
I don't want to block this, I just wanted to point it out and ask.
Right, that was my question. We now get an AutoloadingStorage here. Previously we got a FileStorage, and since FileStorage has it's own static cache, we were not setting a storage cache. Now we have an AutoloadingStorage, which is based on FileStorage and uses it behind the scenes, but we are setting the storage cache as well, since it's not a FileStorage. I was wondering if we needed to. As I was stepping through HEAD and the patch trying to find the difference that caused the bug in listAll it was one of the differences I noticed, as the importer was operating on a CachedStorage object in the patch versus a FileStorage object on HEAD. It wasn't the problem, but I'm wondering if we need the sourceCacheStorage with the Autoloading storage, or if we should carve out the same exception for AutoloadingStorage here that we have carved out for FileStorage.
@deprecated in drupal:11.1.0 and is removed from drupal:11.1.0. Inject
Should this not be removed in 12.0.0?
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co.... Found this too, while I was debugging the tests? Do we need to add the Autoloading storage here?
in the Storage Comparer:
if ($source_storage instanceof FileStorage) {
// FileStorage has its own static cache so that multiple reads of the
// same raw configuration object are not costly.
$this->sourceCacheStorage = new NullBackend('storage_comparer');
$this->sourceStorage = $source_storage;
}
Lol, took forever to find the listAll() bug, and you fixed it before I could push :-)
catch → credited mikelutz → .
mikelutz → made their first commit to this issue’s fork.
This one seemed like fun.
I preserved BC, so if orderby is null, or === 'key', or === 'value' existing behavior is preserved.
if orderby is a string other than 'key' or 'value' it is treated as a key inside the value. I did not require a prefix 'subkey:' as it seemed unnecessary. If you need to reference a key named 'key' or 'value' it can be done using the advanced syntax that follows. To drill down into deeper items in the sequence arrays, separate keys with a '/' (see tests). This mimics the format we use to access nested array items in the migration system.
if orderby is an array, each item is assumed to be a sort directive, and the sequence is sorted by the first, then the second, and so on. The sequence is finally sorted by the element values (while preserving keys) so as to be deterministic.
Each sort directive can be either a string, which is taken to be an element key as decribed above, or mapping containing at minimum a 'sort_by' key (containing the element key described above) and optionally 'sort_order' and 'sort_flags' keys.
sort_order and sort_flags accept as strings the constant names described here: https://www.php.net/manual/en/function.array-multisort.php sort_flags can either be a string containing a single flag, or a sequence containing multiple flags to OR together.
It would be nice if 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work were in, and we could avoid parsing the constants as strings altogether. If we don't wait, we should probably add some error checking here to ensure only the expected strings are used.
A dozenish tests are provided to test combinations of sort orders, ensure that string keys are preserved. and to demonstrate capibilities.
A new `UnsupportedSequenceSortConfigException` is provided and thrown if a sequence element does not contain a key that the schema requests that we sort on.
mikelutz → made their first commit to this issue’s fork.
fjgarlin → credited mikelutz → .
This can be committed with review. The distinction mentioned in #19 is the distinction beteween the plugin setting the value to NULL vs the Executable setting the value to NULL when the SkipProcess Exception was thrown. Personally I don't think there is a distinction because throwing a SkipProcessException set the value to NULL, so I feel like the plugin was responsible for the NULL anyway. But the point is moot, as ✨ Allow process plugins to stop further processing on a pipeline RTBC refactored away the exception and now the process plugin does literally set the value to NULL. I consider the doc change trivial, but it's fine to commit, as it does improve the docs.
mikelutz → changed the visibility of the branch 3336070-skiponempty-is-ignored to active.
mikelutz → changed the visibility of the branch 3336070-skiponempty-is-ignored to hidden.
Also switching this to major based on "The major priority is used for issues that are not critical, but that do have significant impact or are important by community consensus." As recipes are a hot item right now I would consider having the example recipe be functional for 10.3.0 would be important, but feel free to reduce if I am wrong.
Crediting @Josue2591 as he wrote the code. I just opened a new MR against core, cherry picked the commits and (hopefully correctly) fixed some merge conflicts.
Alright, lets try that one.
mikelutz → changed the visibility of the branch distributions_recipes-3447994-3447994-example-recipe-isnt-with-test to hidden.
Posting a relevant slack thread.
Core contrib question: this issue 🐛 Example recipe isn't functional Needs review got moved into Needs Work status by a bot. Is there something we can do to get it back to RBTC?
Participants:
Related Issues:
See https://onlinephp.io/c/faedc
Try catch is like 10x slower than doing most any kind of type checks or conversions. Because explode ends up having to do the same conversions anyway PLUS run the unneeded explode call, and then check the error handler and find the catch block. It's always going to be more efficient to just do our own type checks.
Admittedly removing the type check to the constructor negates the real world impact of this as we are no longer checking the configuration on every row, but a microsecond is a microsecond..
I'd prefer not to use a try-catch here, it's certainly not needed, as we know precisely what explode will accept, a string that is not the empty string. Ultimately I would like to require the configuration item to be a string. It's easy enough to force a string in yml by quoting it. So lets set ourselves up to be strict. Go with this
if (!is_string($configuration['delimiter'])) {
trigger_error('Using a non-string as a delimiter in the explode plugin is deprecated in 11.1.0 and will throw a MigrateException in 12.0.0. Ensure your value is quoted in your migration yaml if necessary', E_USER_DEPRECATED);
if (is_array($configuration['delimiter'])) {
$configuration['delimiter'] = '';
}
$configuration['delimiter'] = (string) $configuration['delimiter'];
}
if ($configuration['delimiter'] === '') {
throw new \Drupal\migrate\MigrateException('Delimiter muse be a non-empty string value.....')
}
parent::__construct($configuration, $plugin_id, $plugin_definition);Move it to the constructor, since this is an error on the configuration and not the value, keep the MigrateException for now. I'm tempted to build something new with annotations and validators, but no time for that now..
The correct check is `if (!is_string($x) || $x==='') throw.. `
mikelutz → created an issue.