This is blocking CI here: https://git.drupalcode.org/issue/drupal-1792310/-/jobs/1725958
What is this limit supposed to achieve?
joachim → created an issue.
> Please check what phpcs config and coder version your test run uses.
This was the update bot that posts patches on d.org.
joachim → created an issue.
I've rebased the MR, it's ready to review.
I've not tried the dev release, but I can see from the MR the problem exists there too.
MR LGTM.
The problem is in Markdown:
public function process($text, $langcode = NULL) {
// Only use the parser to process the text if it's not empty.
if (!empty($text)) {
$language = $langcode ? \Drupal::languageManager()->getLanguage($langcode) : NULL;
$text = $this->getParser()->parse($text, $language);
}
return new FilterProcessResult($text);
}
The parameter to FilterProcessResult is supposed to be a string.
Figured out gitignore.
Made a new branch & MR as the old branch name wasn't accurate any more now that the new functionality is being added to the existing drupal/core-composer-scaffold Composer plugin.
I've hidden the old branch.
New MR is: https://git.drupalcode.org/project/drupal/-/merge_requests/8216
joachim → changed the visibility of the branch 1792310-locations-class-writer-plugin-11 to hidden.
AND it's a parameter that's only used in tests??? That's not a good pattern.
- public function __construct(string $name = NULL) {
+ public function __construct(string $name = NULL, ?string $root = NULL) {
This parameter gets added with no documentation.
This module is not meant to be used on production sites. As such, I don't think it needs security evaluation.
Looks good! Thanks!
joachim → created an issue.
Looks good!
The downside of using a config/install file is that once it's installed, the site owns the configuration, and the contrib module that supplied the config can no longer make changes to it. Install config is good for starting points and templates, but not so good for my use case where the external entities for Unomi are the crucial machinery of the module.
Something that might be of interest is that I've just made a module that combined identically-named fields from different datasources. Unlike aggregation, you pick just one field name in the UI.
The use case is that I have some content entity datasources but also some Date Recur occurrences from event nodes (another module I'm working on, based roughly on this sandbox: https://www.drupal.org/sandbox/sam/3200275 → )
I don't think this is an issue on the latest version -- see 🐛 Blank lines in code get turned into P tags by filter_autop filter Active .
I've managed to get this working, so it was a configuration / documentation problem.
There's a lot of unrelated fixes in that MR which make it harder to understand what's going on.
Things like code style fixes & DI should be done in separate issues, either before or after. (And this is one reason why git commits on feature branches should be small and atomic, because then doing this would be simply a matter of cherry-picking to a new branch, which isn't going to work here.)
Fair enough! :)
I'm not sure when I'll find time to update this, but I'll look at MRs and happy to add a co-maintainer!
There's probably not much use for it any more, as it's very rare that you need to create DB tables directly with Drupal 8+.
Oh wow yeah that used to be in core but was removed ages ago:
commit 708ce0a998fc3f315b66ce629cc021944c2218de
Author: effulgentsia
Date: Thu Oct 1 16:25:03 2015 -0700
Issue #2576533 by alexpott, stefan.r, Wim Leers, dawehner, xjm, effulgentsia, catch: Rename SafeStringInterface to MarkupInterface and move related classes
R080 core/lib/Drupal/Core/StringTranslation/TranslatableString.php core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
I'm afraid I don't follow this at all.
I can *just about* understand the problem at other issue. How was that fixed incorrectly?
+++ b/src/MetatagImport.php
@@ -205,6 +205,18 @@ class MetatagImport {
+ if (!empty($raw_value[0]['value'])) {
+ if (function_exists('metatag_data_decode')) {
+ $raw_tags = metatag_data_decode($raw_value[0]['value']);
+ }
+ else {
+ $raw_tags = unserialize($raw_value[0]['value']);
This could use comments to explain what's going on.
Rebased and fixed the commit -- it was on the 8.x-1.x branch of the fork rather than the feature branch!
Fixed.
> Can you give more information about the sort of "configuration error" you have in mind?
IIRC I was using an XML source and I hadn't set in my migration yml which bits of XML data were keys. Therefore the source plugin returned an empty array of keys.
> 3) fix the monsters in the SQL map around having no destinatiion IDs. Store the source IDs, refuse to look up sources by destination id. If there is code in drush or rollbacks that has a problem with it outside of what we could fix in the map itself, investigate and find solutions.
That might be the most streamlined fix.
However, in terms of DX it's best to be told that your source or destination are currently incorrectly configured.
> In short: if you make a mistake, then bad things happen, Well, that is sort of expected. What we should do in this situation is catch the mistake early and give a helpful error message. It is not clear to me why you want to fix the problem by throwing an exception from ensureTables().
Because throwing an exception from ensureTables() is catching the mistake and giving a helpful error message!
Looks good. Thanks!
Actually, closing this for the more up to date 📌 Refine field descriptions Active .
I'm not sure this needs to be postponed any more - and that other issue may be obsolete with the new field type categories system.
This is maybe overkill. Pondering.
We should at least document that it's fine to mix both in one array, because core does that too.
I'm going to file a follow-up for splitting the settings method into field settings & storage settings.
Tested it locally and all works. Committed.
Thanks for reporting and working on this!
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.