🇩🇪Germany @Grevil

Ok, the current implementation doesn't work anymore. "migrationsList()" is already implemented, and once removed, the "_construct()" method throws the next error, expecting less parameters, and the list probably goes on after this, seeing the changes made by @imash in 6.0.x. But those changes seem too much? Unsure about this. Either way also using the updated 6.0.x by @imash and adjusting the namespace also throws an error when migrating:

[error] Error: Call to a member function getMigrationDependencies() on array in Drupal\migrate_tools\Drush\Commands\MigrateToolsCommands->importParallel() (line 1022 of /var/www/html/web/modules/contrib/migrate_tools/src/Drush/Commands/MigrateToolsCommands.php) #0 [internal function]: Drupal\migrate_tools\Drush\Commands\MigrateToolsCommands->importParallel()

And generally I don't think this is the correct approach for parallel migration, as we implement a new standalone "migrate:import-parallel" command, instead of extending the existing "migrate:import" command and adding a "--parallel" flag. I understand that this isn't easily implemented, but in the current implementation we drop crucial flags like "--continue-on-failure" or "--group=...", which at least for us, is a no-go.

grevil → changed the visibility of the branch 6.0.x to active.

Sorry, that patch was empty...

Rebased the issue forks issue branch.

Here is a static patch from the rebased MR.

grevil → changed the visibility of the branch 6.0.x to hidden.

Seems to work great! Currently, in the midst of migrating a large Drupal 6 site. If anything goes wrong related to this module, I'll comment here again!

Note, that the introduction of the new Decorator leads to an error when migrating d6 nodes. But that is unrelated to this module but rather a drupal core issue. I just created an issue + fix here: 🐛 "core/modules/node/src/Plugin/migrate/source/d6/Node.php" "__construct" method uses variable of type "ModuleHandler" instead of "ModuleHandlerInterface" Active .

Otherwise, RTBC +1!

Simple but effective change. Please review!

grevil → created an issue.

Providing static patch for the time being.

Ok, the js preprocessing (aggregation behavior) was overridden in our local dev environments "settings.local.php", meaning I have never seen any js / css aggregation in Drupal ever.

I adjusted the tests slightly, so that they should run theoretically now, but they don't.

As @lrwebks correctly mentioned, we can't check for any files directly if they are aggregated. This Test works in other cookies submodules, because there we manually disable any js aggregation, so we can morph the "data-src" attribute, but we don't need to do that here (see @anybody's example code here).

I disabled the file check, so now a different error appears:

Drupal\Tests\posthog_cookies\FunctionalJavascript\PosthogCookiesFunctionalJavascriptTest::testPosthogCorrectlyKnockedWithJsAggregation
TypeError: Cannot read properties of undefined (reading 'init_config_json')

Sounds like a racing condition to me? Maybe also something different. But I just tested it with ACTUAL js aggregation in my test site and the module works fine with js_aggregation enabled. So its a test error.

Ok, unsure how relevant this is, but the original issue is NOT fixed.

The current behavior is, that when we consent, the posthog cookie appears as expected. If we then deny through the UI again or fire:

var options = { all: false };
document.dispatchEvent(new CustomEvent('cookiesjsrSetService', { detail: options }));

To deny, the posthog cookie disappears as expected (and as the newly added test replicates).

BUT if we simply remove the cookiesjsr cookie and the posthog cookie, and then reload, the original issue reappears and the posthog cookie gets set again. Only if we remove it again and then reload, it disappears.

How relevant is this @anybody? Should we eventually always set the cookiesjsr cookie on page load (With denied behavior as default) instead of only setting it, once we accept / deny?

We should also consider the option that the test is correct and with aggregation enabled posthog isn't currently handled correctly

On a real site, everything works as expected. Only the test fails.

I am unsure, if this is related, but in line 81 of the core `core/modules/file/src/Plugin/migrate/process/d6/FieldFile.php` process-plugin a hard coded `d6_file` is used for the migration lookup:

    $lookup_result = $this->migrateLookup->lookup('d6_file', [$value['fid']]);

Unfortunately, this will cause a problem when migrating d6 file field instances, as the migration id "d6_file" is not resolvable, when using "migrate_upgrade" as the actual migration id is prefixed with "upgrade_". Meaning, once migrate_upgrade is activated, this id should get overwritten with "upgrade_d6_file" instead:

    $lookup_result = $this->migrateLookup->lookup('upgrade_d6_file', [$value['fid']]);

But this would probably need changes in the core file first?

We should see if the external endpoint is also affected and other modules (e.g. friendly captcha) are affected as well. I'd suggest, if further captcha submodules are affected, we should introduce a setting in the parent "captcha" module. If only friendly captcha (or only the internal endpoint) is affected, we should fix it in friendly captcha.

Internal suggestion by @anybody:

_maintenance_access: Set this to TRUE to bypass the offline message and show the page normally while in maintenance mode.

grevil → created an issue.

@lrwebks, was this ready for rereview again?

This sounds like a major problem, I'll come back to you asap.

Thanks, @rhovland!

I'll come back to you asap, putting this issue and 🐛 WSOD on order completion page when order has items without purchased entites Active on my list!

Ok, I tried to explain my thoughts for most of the changes in the MR and added a few questions. This is not final yet.

The tests are still failing, and the template needs some more love. Furthermore, I added a few questions in the MR.

grevil → created an issue.

Works great again. Weird, that the default is not taken from the parent class.

This caused a regression, see 🐛 Undefined array key "fences_field_items_wrapper_classes" Active .

Released 3.0.7 → . Sorry for the delay for such an annoying issue.

Yea, no idea how that got in there, but there is neither a "fences_field_items_wrapper_tag", nor a "fences_field_items_wrapper_classes" key. Instead, they are called "fences_field_items_wrapper_tag" and "fences_field_items_wrapper_classes" (as @guilhom already stated).

This was introduced through ✨ Show fences settings in field settings summary Active .

grevil → created an issue.

We encounter the exact same issue now. Quite annoying...

I'll have a look later on today!

Done! Please review!

grevil → created an issue.

@musa.thomas you can simply go to the latest commit via the gitlab UI and add a ".diff" at the end of the URL. This will give you the plain diff (patch). Then you can simply add this patch as you desire (e.g. through using cweagans/composer-patches).

I commented in the other issue, how I got a D7 to D11 media_migration to work. It is a hacky workaround, but it seems to do the job just fine:

Media Migration Patches:

• The D11 compatibility patch
• The dirty early return fix, avoiding an exception, which stops the migration

Drupal core Patches:

• This core patch to revert the array_map changes done in core →

OK, to quickly summarize my actions here:

NONE of the issue branches properly fix this problem!!!

I did a "successful" migration to D11 using the following patches:
Media Migration Patches:

• The D11 compatibility patch
• The dirty early return fix, avoiding an exception, which stops the migration

Drupal core Patches:

• This core patch to revert the array_map changes done in core →

This is definitely not the proper way to do this, but it seems to do the job just fine. The real cause of this issue still needs to be found.

grevil → changed the visibility of the branch 3494209-drupal-7-to to hidden.

Basically, we want to return a simple Json response in the example that looks a bit like this:

{
	"version": "1.0",
	"type": "video",
	"provider_name": "YouTube",
	"provider_url": "https://youtube.com/",
	"width": 425,
	"height": 344,
	"title": "Amazing Nintendo Facts",
	"author_name": "ZackScott",
	"author_url": "https://www.youtube.com/user/ZackScott",
	"html":
		"<object width=\"425\" height=\"344\">
			<param name=\"movie\" value=\"https://www.youtube.com/v/M3r2XDceM6A&fs=1\"></param>
			<param name=\"allowFullScreen\" value=\"true\"></param>
			<param name=\"allowscriptaccess\" value=\"always\"></param>
			<embed src=\"https://www.youtube.com/v/M3r2XDceM6A&fs=1\"
				type=\"application/x-shockwave-flash\" width=\"425\" height=\"344\"
				allowscriptaccess=\"always\" allowfullscreen=\"true\"></embed>
		</object>",
}

(https://oembed.com/)

Except, that we won't use the legacy "object" element in the html, but an iframe instead (with the "sandbox" attribute of course). Checkout "WidgetJsonExample" for reference.

Ok, cleanup finished.

grevil → created an issue.

grevil → created an issue.

Or actually postponing is probably better.

Closing this as a duplicate then.

This issue also fixes 🐛 Field formatter with inline settings does not provide third_party_settings form of target field Needs work .

Yep, works out of the box using the issue fork from 🐛 Field formatter with inline settings is missing field formatters from the referenced (media) entity type Active :

Great stuff :)

This is probably fixed through 🐛 Field formatter with inline settings is missing field formatters from the referenced (media) entity type Active .

The "Field formatter with inline settings" never used the "real" field definition, but instead created a new one, meaning some field definition settings were reset to their default values (and third_party_settings probably got ommited entirely).

Please test, once that issue is merged (or test the issue fork directly).

POSTPONED on 🐛 Field formatter with inline settings is missing field formatters from the referenced (media) entity type Active .

Ah sorry, it says in the title, that you are using the "Field linker" formatter.

Hey, are you using the "Field formatter with inline settings"?

The solution in 🐛 Field formatter with inline settings is missing field formatters from the referenced (media) entity type Active could possibly fix your problem. The "Field formatter with inline settings" never used the "real" field definition, but instead created a new one, meaning some field definition settings were reset to their default values.

Just did some internal testing and seems to work great! 😁👍

This should do the trick. Now we are getting the "real" field definition, instead of creating one.

Ok, I found the issue.

Instead of using the actual "real" field defintion of the referenced entity, we create a new one from the given storage definition here.

In the case of the file field, this will lead to "$field_definition->getSetting('file_extensions')" having the default value: "txt", instead of the actual "mp4" value. This in turn, leads to "isApplicable" returning false and the formatter not being displayed.

I will have a deeper look. This is caused by how the apllicable formatters are loaded. For each formatter "isApplicable()" is called, but both the Vidstack and Video formatter are extending the core "FileMediaFormatterBase". And their implementation of "isApplicable" fails, because the doesn't apply here, because the "FieldFormatterWithInlineSettings" field definition "file_extensions" setting doesn't include the media mime type.

grevil → made their first commit to this issue’s fork.

As just discussed on Slack, changes in https://git.drupalcode.org/project/responsive_favicons/-/merge_requests/10 LGTM! Good stuff this module FINALLY gets the love it deserves (even if we don't use it anymore). 😁🎉

I agree, this needs clear steps for reproduction.

This is easily reproducible, as seen in the first screenshot of my comment #6. But as stated in my previous comments, I don't think that the current implementation is incorrect, nor is it an issue on our behalf.

This should do the trick and prevent BC.

Yes! And thank you for the release! 🎉

grevil → created an issue.

Thanks for finding this! Although, the current change would introduce a breaking change, as we would never enter the switch case. Currently, only the "default" case is problematic the other two are fine.

Although, I am starting to wonder where these settings are even coming from and if we really need them? There isn't even a settings page to set any of these. The code is from me, but I don't remember what this is for. I'll need to have a deeper look in a separate issue.

For now, we can temporarily move the early anonymous return inside the default case to avoid any breaking changes, in cases where someone set the "username_display_override_mode" manually via config. NW until adjusted.

Yes, thanks.

As a middle ground, we could also set the error on both the mail address and password field, as it currently also shows an error on the "Log in" button, which is understandably quite bad... But I still see the issue in "inline_form_errors" and not our side. Again I don't think implicitly setting form errors on form element children is an expected behaviour...

What do you think @anybody?

This is the current IS, when inline form errors are enabled:

And this is the IS, when it is disabled (working as expected):

Unsure about this fix. The purpose of this error is to highlight the whole wrapper, instead of only the username, because the problem is either the username OR the password can be incorrect here and we don't want to let the user know which field is incorrect, to avoid brute force attacks.

With:

When the inline for[m] error is enabled, [...]

you mean the drupal core "inline_form_errors" module? I would cautiously say, that it is unexpected behaviour, that this module recursively goes through all child elements of a wrapper and displays the same error for each?

I'd say you create a core issue for this instead. I will set this to "works as designed" for now.

All green. Merging.

All green after rebase. Merging.

Yep, that should be all, thanks again! LGTM! 🎉

Just ran into the same issue! Thanks for providing the fix! 😁👍

grevil → made their first commit to this issue’s fork.

Test failures are unrelated. As of #9, they just recently succeeded.

We need to adjust the commerce composer requirements in a follow-up issues to let the tests run again.

Thanks all!

grevil → made their first commit to this issue’s fork.

LGTM!

I published 3.0.5: https://www.drupal.org/project/glossify/releases/3.0.5 → .

Thanks everyone!

Changes LGTM! Great Test coverage! 🙂👍

grevil → made their first commit to this issue’s fork.

All green. Merging.

Did some adjustments to the wording. Otherwise, LGTM!

grevil → made their first commit to this issue’s fork.

Nice work! I added a few comments. 🙂

I only looked at the code yet and haven't manually tested the module in our dev environment. So once everything is resolved, I can start doing that! 👍

The "Add new comment" section has a details wrapper further below the textarea, called "Credits & commiting" where you can give credit to people working on an issue:

Could you adjust the credits accordingly and create a new release with the changes? :)

Great stuff, thank you @rhovland!

Done.

grevil → created an issue.

grevil → changed the visibility of the branch 3506406-opt-into-security to hidden.

Done.

grevil → made their first commit to this issue’s fork.

Maybe we could get a subsystem maintainer to have a look at the tests, since nobody dares to do the final refinement and this is open for a while now?

Alright, that should be it. Test is still failing, so leaving this at "Needs work" for now.

Current MR as patch attached.