Made a few tweaks.
Could you check to see if it still fixes the problem for you?
joachim → created an issue.
joachim → created an issue.
Thanks!
Rebased on 11.x
Ah, I should have upped the minimum version, sorry!
The change is fine for 10.2 and newer -- see https://www.drupal.org/node/3395582 → .
joachim → created an issue.
I wrote code to do this in https://www.drupal.org/project/unomi_connect → , in the MR for issue 📌 Use external_entities to declare Unomi types as Drupal entities Active . See https://git.drupalcode.org/issue/unomi_connect-3376616/-/blob/3376616-us... in particular.
Nearly there -- looks good overall, just a few formatting fixes needed.
We should combine PHPCS with Rector.
For example, when running the Rector rule to convert annotations to attributes, the resulting PHP attribute is all on one line and the classes aren't imported. That's because Rector can't do class imports (it can only modify the node it found AFAIK) -- I don't know whether we could improve its output to do linebreaks.
But the resulting code can be improved by then running it throuh PHPCS & PHPCBF. In the case of converting annotations to attributes, the Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing sniff will fix the classes at least.
So Rector rules should come with a list of the specific PHPCS sniffs that are needed to correctly reformat the fixed code, and our tooling should run Rector AND PHPCBF with those sniffs.
Thanks! MR looks good.
I've committed 🐛 Computed field should specify the cardinality of the field Fixed so this needs rebasing again.
Re #3, is it, but this issue in on the 3.x branch and 🐛 Computed field should specify the cardinality of the field Fixed is on 4.x.
Made a few changes, added a test.
Committed the MR.
The 8.x-1.x branch has only that one commit on it that is not also on 2.0.x, and that commit is a cherry-pick. I imagine the committer didn't check the support status of the branch.
The core plugin needs some changes before it can be used, but let's see what the Migrate maintainers say on that issue.
And yes, like that:
destination:
plugin: devel_null
Documentation should also say that nothing is saved to the map.
Ah yes, that commit is 🐛 Upgrade filter system to HTML5 Fixed and there's this:
> The HTML5 normalizer normalizes the non-breaking space character to , I think this is OK and certainly easier to see when reading HTML output.
Ah, core commit 201ae2e35438b7d8f7c831ba8ac33bfc035bbb0a looks like a possible suspect:
$f = (string) $filter->process('<code onerror> ', Language::LANGCODE_NOT_SPECIFIED);
$this->assertNoNormalized($f, 'onerror', 'HTML filter should remove empty on* attributes.');
- // Note - this string has a decoded character.
- $this->assertSame(' ', $f);
+ $this->assertSame(' ', $f);
> The null destination plugin is intended to throw errors and not meet requirements.
Is it for tests? If so, that should be in a test module, and with a clearer name too.
A dev/null destination plugin is useful for developing migrations, so the 'null' destination should be made usable -- I filed ✨ add a null destination Needs review because I hadn't realised this class existed in core.
My use case was that I was trying to understand how to get data from a source that was a large set of XML files, in structured folders. I wanted to check that my configuration of the Url and XML source plugins (from Migrate Plus module) was working, and to see how much custom code I would have to do. To do this, I needed to run the migration and see the data (with Migrate Devel module's debugging options for Drush). But I didn't want to have to set up a real destination -- partly because that would be extra work, and also because I'd then keep having to roll back to run the migration again, because the map would get saved. So a null destination was exactly what I need -- it allows the migration to be run, does nothing with the data, so you can run it over and over again while you develop iteratively.
The 8.x-1.x is not supported, and there are no releases on it.
The steps to reproduce are already in the issue summary.
Turns out this is in core already!
Although it does:
> requirements_met: FALSE
Doesn't that mean migrations won't run with this?
Huh, there's a NullDestination? I just made a MR for migrate_devel with precisely that -- it was in fact how I discovered this problem, because the map and message tables go haywire.
Have a look at my MR at ✨ add a null destination Needs review -- grab the code from that to change the core NullDestination.
Thanks! Looks good.
One small typo. Other than that looks good.
I am not entirely sure about silently ignoring an 'sql_mode' in 'init_commands'. The documentation for the whole of this is pretty sparse, so someone might reasonably see 'init_commands' and try to use 'sql_mode' in that since it is an actual MySQL init command. Might be better DX to throw an exception so a developer is immediately told they've done something wrong, rather than leave them headscratching over why it doesn't work.
@darvanen are you looking at the right branch?
In refs/heads/2939760-10.3-allow-granular-overriding-of-sqlmode-options:
@trigger_error("The 'sql_mode' database command is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use an array of options in 'sql_mode_options' instead. See https://www.drupal.org/node/3403416", E_USER_DEPRECATED);
joachim → created an issue.
Well spotted!
Fixed.
Rebased on 11.x and resolved the conflict.
This is good for novices again -- see #5 for what needs doing.
Someone should rebase the branch before a novice works on it -- it looks like it's been merged rather than rebased, which is going to make the history messy.
MigrateSkipRowException lets you define whether to save the skipped row to the map or not:
public function __construct($message = '', $save_to_map = TRUE) {
The new API should allow that distinction too.
That's potentially out of scope of this issue and left to a follow-up, but it should happen before we deprecate MigrateSkipRowException as otherwise it's a regression.
That seems weird -- the skip_on_value plugin returns the incoming value when it doesn't skip. I would debug to make sure what's going on (with the migrate_devel module's process plugin for example).
You can also use a dummy destination for a skip process plugin, e.g.
process:
dummy_field_nothing_will_be_saved:
-
plugin: skip_on_value
method: row
equals: true
value:
- myvalue
source: myfield
joachim → created an issue.
You need to do something like:
process:
myfield/0: source_alpha
myfield/1: source_beta
(I can't remember the exact syntax for setting deltas, but it's something like that.)
joachim → created an issue.
> * 'ONLY_FULL_GROUP_BY' => true,
Should be TRUE.
Could that be the failure? I don't know if the PHP linting checks @code blocks in docblocks.
We need something like this in DataExport where the filename is obtained:
$tokens = $view->build_info['substitutions'];
$filename = $this->viewsTokenReplace($text, $tokens);
However... DataExport is called statically, and viewsTokenReplace() is not a static method.
@guignonv Basically, at the moment the module is written to put the ability to define external entity types in the hands of the site admin, by creating the entities in the admin UI.
I'd like it to be possible to allow third-party modules to also define external entity types.
So that, for example, you install Unomi Connect module, it installs External Entities as a dependency, and the Unomi external entity types are automatically defined when you enable them.
Nearly there!
joachim → changed the visibility of the branch 2939760-9-allow-granular-overriding-of-sqlmode-options to hidden.
joachim → changed the visibility of the branch 2939760-10.1-allow-granular-overriding-of-sqlmode-options to hidden.
Hi.
It looks like you've taken the whole of the patch from the other issue. We only want the docs, and not all of the docs.
Thanks for the update.
The link to create a new issue in the Drupal core issue queue has gone as well -- presumably because it was output as part of the comment form. That was really useful though.
It's now been 6 months...
> Not being able to comment is less broken than not being able to read the documentation...
+1
Comments seem to not be working at the moment anyway?
The source configuration is going to get more complicated with changes like ✨ Allow wildcard for URL source plugin Needs work and ✨ Allow callback for Url source, and single item Json plugin RTBC .
re #28 again... ah now I've read more of this patch and the patch at the other issue, I see how they could work together.
✨ Allow callback for Url source, and single item Json plugin RTBC should be changed so it defers executing the callback, and that's when this MR should run the globbing too -- in neither case should we be spookily changing the plugin's configuration, that's just confusing DX!
-
+++ b/src/Plugin/migrate/source/Url.php @@ -32,11 +71,23 @@ class Url extends SourcePluginExtension { + $configuration['urls'] = $configuration['urls']['callback']($migration);Rather than load up a potentially HUGE list of URLs into the configuration, we should defer this to getSourceUrls(), and the fetchers should call that.
Also, changing configuration feels a bit ick -- it should remain what it was set to in the plugin definition rather than being spookily changed.
Found a problem when the URL doesn't use a stream wrapper like public:// and is intead an absolute filepath.
Working on fixing it...
re: #28, allowing callbacks might be useful too, but globs are a useful tool that we have in a filesystem which we don't have for URLs, which make a declarative definition possible. So I think it makes sense to have them too.
Made a MR with a few small fixes.
Tagging as novice.
There are docs in the patch at ✨ Allow callback for Url source, and single item Json plugin RTBC that should be extracted and used here -- omit the final section about the callback property, since that is new in that issue.
> So if you have a fixed hook then you could just create a attribute class and put the API documentation on the constructor. A somewhat small change to api.drupal.org, hopefully
Harder to discover by reading code if hook attributes are mixed in with other kinds of attribute in src/Attribute.
This now seems to work if we adapt ListBase to handle providers that are not a string (WTF migrate????!).
Rebased.
I doubt it would be simple, but it could build on top of the reverse reference computed field that's provided by https://www.drupal.org/project/computed_field → .
Oh yes, we need to rethink MODULE.api.php as part of this issue. Otherwise it's not going to make sense.
> #[Hook('hook_form_FORM_ID_alter', form: 'myform')]
> #[FormAlter('form_id')]
I'm not hung up on the specific syntax, the point was splitting out the variable part for the FORM_ID placeholder into a separate value.
Happy to leave it as a follow-up to keep this issue simple.
What I meant is, would we have:
#[Hook('hook_form_myform_alter')]
or split this out to a parameter:
#[Hook('hook_form_FORM_ID_alter', form: 'myform')]
How will hooks whose names include a variable work?
E.g. hook_form_FORM_ID_alter, hook_ENTITY_TYPE_update()?
Could the variable part be a separate value in the attribute?
Argh. I was looking at https://github.com/spatie/laravel-ignition.
What's the difference between the two?
> What is the impact of having more than one attribute? What actually breaks?
Nothing breaks, but only the first attribute is read:
if ($attributes = $reflection_class->getAttributes($this->pluginDefinitionAttributeName, \ReflectionAttribute::IS_INSTANCEOF)) {
/** @var \Drupal\Component\Plugin\Attribute\AttributeInterface $attribute */
$attribute = $attributes[0]->newInstance();
This change is just for DX, so developers understand why the code they've added does nothing.
Thanks for the MR.
LGTM!
I would have started a new issue in book contrib for this.
This wasn't a core issue, it was an issue about deploying code on d.org, and it was a total waste of time.
Pushed the work I did on 📌 Convert entity type discovery to PHP attributes Needs review to a MR.
Still to do:
- > unless there is a deprecation marked.
Bikeshedding: what do we call these?
- I initially went for 'extending plugin attributes' but I think that's misleading because there is no class inheritance going on
- '3rd party' doesn't apply, I feel, because with config 3rd party settings, those properties are completely controlled by another module - they are set, and read. Here, the properties are set by modules that implement plugins, and read by the module providing the extra attribute
- additional plugin attribute?
- supplemental plugin attribute?
joachim → created an issue.
We were saying earlier that the next steps should be an issue in core, and a discussion about what the API should look like.
Making an MR is rather jumping the gun.
I think that requiring an additional form element isn't good DX. I'd rather do something like add a '#select_all' => TRUE to the existing checkboxes element.
But as said, this should be done in core, not here.
Yup.
IIRC, this was only developed because it was quicker than doing a patch for core, and back in the D7 days a new feature like this wouldn't have got in anyway.
That sounds like a good roadmap to me.
One small problem is that I'd thought it would be good DX if supplementary attributes were not able to redeclare a property that's in the main plugin attribute:
$attribute_properties = $attribute->getArguments();
foreach ($attribute_properties as $name => $value) {
if (property_exists($content, $name)) {
throw new InvalidPluginDefinitionException("May not reuse $name.");
}
}
If we later on want to move a property like field_ui_base_route to a supplementary attribute, we need a way to make an exception to that rule, for BC handling, because there will be a period when the property is in both the main attribute and the supplementary attribute.
That could fairly easily be done by marking the property with an attribute to say it's expected that it does that, but it's an extra piece of complexity.
Do we really need tests for something that's going to be removed from 11.x?
> Prioritize (like core initiative style) efforts to rationalize, standardize and clean-up our existing tests.
That could come under the 🌱 Boilerplate reduction initiative Active which I am trying to get up and running.
> The tests for Views module, for example, are completely whack.
Yes! And the setup that the tests do to create views is crazy as well.
> Has this "custom code" been published somewhere?
No, sorry. And I've changed machines twice since that project now, so it's very likely it's gone.
It was fairly straight forward though -- create the entities, set the values, save them. There might have been some things to do to put it into the cart -- look at the Commerce add to cart forms to see the API.
Made a 10.3.x branch, so we have:
- https://git.drupalcode.org/project/drupal/-/merge_requests/7739 - 10.3.x
- https://git.drupalcode.org/project/drupal/-/merge_requests/7207 - 11.x with deprecation removed
> There are a bunch of other related to images, I suspect there is an indirect call missing in the testing somewhere.
Nothing as complicated as that, just that last night I was tired and I forgot to also replace the
>createImageField\(([^,]+), ([^,]+),
>createImageField($1, 'node', $2,
uses as well :/
Argh I did a search and replace of the whole codebase! I could have SWORN I did that, and I don't see the commit!!!!
In case I have to do this again, the search and replace is:
>createImageField\(([^,]+), ([^,]+)\)
>createImageField($1, 'node', $2)