🇩🇪Germany @Grevil

Yea, we explicitly do not want the suggested change as the comment says.

Alright, done, please review!

Ok! Let's solve this in 📌 Implement posthog_events submodule to track user actions Active .

What do you mean by

updated their credentials

Probably "hook_user_update()". But yea, I still feel a LOT of these hooks have minor importance, but yea it's basic boilerplate code, so why not.

>Re #4: Maybe we should simply document this below the snippet and link to the posthog documentation? And maybe inform users about available integrations in Klaro and COOKiES? (That don't yet exist, right?).

You are saying we should document this below the json_init field? Unsure about this. Unsure about this. Maybe we should link to the help page below the field and add the information to the help page?

So we probably don't need it for now?
For the time being, anyone who needs it, can simply adjust the "init_config_json" to use a different persistence setting. I am even unsure, if we should provide a setting for this at all. The cookies + klaro integration should be enough for 70% of all cases and the rest can simply change the persistence in "init_config_json".

Follow-up issue for anonymous users here: 🐛 Identify / Synchronize anonymous users client / server side Active .

grevil → created an issue. See original summary → .

POSTPONED until it becomes available for the PHP SDK!

I am not really seeing the benefit of creating custom events for all of these user hooks. Why do we care if a user is blocked, unblocked, has updated their credentials, has deleted their account etc.?

Currently, the user is identified on login server / client sided, and that's most important. I understand the use of custom events for specific sites (e.g. how many people bought product a, how many bought product b), but not here? Let's discuss this first, before implementing.

Let's keep it at "posthog_php_events" for now. We could also rename it to "posthog_events", to also have access to the js.

Currently, we identify a user, once they log in. I guess that should do the trick?

I think that is even better, since we only track the users who register AND also log in.

Merging, but should still be reviewed afterwards!

Damn, seems I already merged some changes in 1.x... I'll revert those.

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

Created the follow-up issue here: 📌 Set metatag parent programmatically (inherited value) Active .

grevil → created an issue.

All done, please review.

Yea, this whole endeavour was caused, because programmatically setting a metatag value to an empty string will kill the metatag output entirely, resulting in the same behaviour as providing <none> (which is the root cause of my confusion).

Only if we set an empty string via UI, the submit call will silently override our empty string with its parent value. That is also, why the tests failed originally. This is REALLY confusing! ESPECIALLY if there is also an empy / isNull check on the ouput handler and no way to tell, that $this->value will silently get overriden on submit...

If we allowed to set an empty string, inherit the value from the parent and delete the metatag, @thomas.frobieter, and I suggest having the following changes:

• Add the ability to provide <none> as value, to kill the metatag entirely.
• Add the ability to provide <empty> as value, to let the metatag have an empty string.
• Add the ability to provide to leave the input empty to use the parent value (inherited).

Note, that the inherit / empty string functionality could also be implemented via checkbox.

To be honest, I don't quite get this request and how the inheritance works for metatags?

Based on the request, it sounds like, if we would set the metatag value of a child metatag to an empty string (e.g. setting the 403 canonical_url to ""), the metatag would use the parents value (global's canonical url). But this isn't the case.

Setting an empty string instead of already does the exact same thing. This is also represented in code and the currently failing tests.

    // If there is no value, just return either an empty array or empty string.
    if (is_null($this->value) || $this->value == '') {
      return [];
    }

    // Support the "<none>" option to hide all output for this meta tag.
    if (!is_array($this->value) && $this->value == '<none>') {
      return [];
    }

And how does the whole inheritance work then?

403

Inherits meta tags from: Global

I don't think it does.

Works great! Thanks for the fix @damien!

I basically copied the example by @anybody, and it works like a charm! The canonical url will get removed on 403 pages if we declare it as .

I'll add a test for this.

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

No, this was correctly adjusted for the D11 release:

    if (version_compare(\Drupal::VERSION, '11', '>=')) {
      $uploadValidators['FileExtension']['extensions'] = $originalExtension;
    }
    // @todo Remove this, once we drop the Drupal 10 support:
    else {
      $uploadValidators['file_validate_extensions'] = [$originalExtension];
    }
  }

Closing this.

Definitly!

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

@jsacksick Indeed! But we ended up in commerce, since we unintentionally allowed a quantity of "0". The original entry point was "commerce_api".

The issue from @Anybody and I would be solved in 🐛 Error: Call to a member function getEntityTypeId() on null Active . We wrongly allowed to set a quantity of "0", which resulted in the error.

I will close the !MR 27, just had a look at 🐛 Price calculation fails with a fatal error on zero quantity order items that have adjustments Needs review . And there seems to be a use case for having a quantity of 0.

Same problem as in 🐛 Error: Call to a member function getEntityTypeId() on null Active . Would see this as a duplicate.

All done, @jsacksick should decide, which approach to use here!

Alright, this should be it for the time being. I am unsure whether this is the correct approach or if we should allow a quantity of "0" and simply add a NULL check in line 183.

I also thought about implementing the check in the "EntityValidationTrait" "validate()" method, but that class seems really general and not really meant for order item validation, so I dropped the thought again.

grevil → changed the visibility of the branch 3495551-error-call-to to hidden.

Wrong module.

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

Ok. @Anybody and I just took a deeper look at this, but we still couldn't find the reason. The error didn't lead to an order cancellation. The customer still bought the products. We tracked down the order and tried to replicate everything he did and more, but this didn't lead to anything. Everything worked as expected.

@Anybody agrees, that the error probably has something todo with the "fake_shipments" array. We adjusted the code internally, to also dump the "fake_shipment" and "fake_shipments" amount. This will give us more insights, once it happens again.

Whoops, my apologies.

But I can not reproduce this issue at all.

My suspicion is, that this has something to do with the fake_order / fake_shipments, commerce_cart_estimate creates in their estimate method.

It basically duplicates the current order and saves it as "fake_order" and packs it through the shipping order manager, resulting in a "fake_shipments" array. Later it goes through the fake shipments and individually calculates their rates and if the resulting "rates" array is empty, the error is thrown:

No applicable shipping rates for the following partial address: (Country code: %s, Postal code: %s).

(See Line 114 and following in "src/Estimator.php")

But at least in our project this error only occurs for Austrian customers, which is quite weird. But since this isn't reproducible, we should postpone this issue, until we gather more information.

Ok, nevermind actually. There are too many markup changes inside https://github.com/jfeltkamp/cookiesjsr/pull/42/files, so we would need to readjust the tests AGAIN, after that issue is merged.

Postponing again. This time on https://github.com/jfeltkamp/cookiesjsr/pull/42/files

Note, we should add an update hook, letting the user know about the BC concerning the adjusted HTML (if anyone adjusted the cookies banner).

Alright! From the discussion in 📌 Review markup / CSS before 2.0.0 Active , we don't need the extra span anymore, so this can go active again!

Ok, so we don't need the span anymore? Then I'll just adjust the tests accordingly and that's it :)

I can't reproduce this issue, but I DO get a validation error, but a different one:

Postal code field is not in the right format.

But that issue would be a https://www.drupal.org/project/address → issue instead.

See

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

The test failures are related to the external cookiesjsr svelte library (see my comment here: https://www.drupal.org/project/cookies/issues/3496407#comment-15941167 📌 Review markup / CSS before 2.0.0 Active ).

Let's postpone this issue on 📌 Review markup / CSS before 2.0.0 Active right now. Afterward, we can see what the status of the tests is.

FYI according to the tests, the "bannerText" should be inside a span with the class "cookiesjsr-banner--text":

    $session->elementTextEquals('css', '#cookiesjsr > div.cookiesjsr--app > div.cookiesjsr-banner > div.cookiesjsr-banner--info > span.cookiesjsr-banner--text', $cookiesTexts->get('bannerText'));

But currently, the text sits directly inside "div.cookiesjsr-banner--info". Just a heads-up.

Agreed!

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

Seems to work great on v3! I fixed the remaining cspell issues and now the pipeline also succeeds again! 🙂👍

I'm just not sure, how backwards-compatible it is.

It's not. We need to drop D9 compatibility. But I am unsure, why we even kept it in.

Yea, pretty simple fix! Seems like we generally had a miscommunication in https://www.drupal.org/project/search404/issues/3434362#comment-15583056 📌 Automated Drupal 11 compatibility fixes for search404 Needs review .

All green! Time to merge.

All done, pretty straight forward, no need for review.

Let the pipeline review.

All done, pretty straight forward, no need for review.

Let the pipeline review.

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

Internally RTBC'd by @anybody.

grevil → created an issue.

Nice generic test succeeds for both 10.3 and 11!

The js test failure will be solved in a follow-up issue.

I think I found something. The fix will be introduced through 📌 Implement SDK base integration Active .

HA, no way. We actually run into 🐛 Drupal 7 to Drupal 11 migration runs forever Active through the tests now!

ERROR: Job failed: execution took longer than 1h0m0s seconds

@heddn, so it actually makes sense now, to merge 🐛 Drupal 7 to Drupal 11 migration runs forever Active and this issue!

Test failures are related to a regression caused through 🐛 "[warning] Drush command terminated abnormally" as soon as posthog_js is installed Active .

This fix caused a regression for Drupal 11. We should make separate releases for D10 and D11:

PHP Fatal error: Type of Drupal\posthog_js\Form\SettingsForm::$typedConfigManager must be Drupal\Core\Config\TypedConfigManagerInterface (as in class Drupal\Core\Form\ConfigFormBase) in /var/www/html/web/modules/custom/posthog/modules/posthog_js/src/Form/SettingsForm.php on line 0

This should be it. Let's wait for entity_embed to get official D11 compatibility.

RTBC, works as expected!

Confirming RTBC and fixed typo

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

🐛 Drupal 7 to Drupal 11 migration runs forever Active isn't finished yet anyway and the current MR there, doesn't solve that issue as explained in the comments.

Yea no idea. I haven't decorated a symfony service before, so I am unsure what I am doing wrong here.... https://symfony.com/doc/current/service_container/service_decoration.html is not very helpful to be honest.

The current implementation should be correct, as we autowire the class_resolver as the constructor argument, but I still get the following error:

[error] ArgumentCountError: Too few arguments to function Drupal\layout_builder_st\DependencyInjection\DecoratedClassResolver::__construct(), 0 passed in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 and exactly 1

Even if I do it the non autowire way, like described in the docs, I get the exact same error.

If I add: class_resolver: ~ I get an error, that the decorated class couldn't get found (which makes sense, as we delete it, but its in the docs for some reason).

@phenaproxima do you have experience with decorating symfony services? Could you help me finish this?

Already done through 📌 Finish and test posthog_js Active . Only issue is 🐛 PH cookie gets set again after manually removing both the ph cookie and the cookiesjsr cookie and reloading the page ( Active , but everything else works great!

Added to merge train.

Thx for the clarification. Merging!

LGTM!

Commented one question.

Great stuff @simonbaese! Thanks for the superfast reply / fix! 🎉

Alright. Adjusted accordingly. I removed the hard pipeline failure for phpcs and phpstan, but I can revert that change again, if you disagree and want them to make the pipeline fail.

Back to needs work, based on the GitLab comment.

@simonbaese, @Anybody and I require this module for Drupal 11, as we just finished a Drupal 7 to Drupal 11 migration, where this issue can happen as well.

So it is indeed still relevant!

Nice, thx!

LGTM! One tiny comment, after that RTBC. Have you tested the update hook?

LGTM!

Ok, to summarize this. The current MR has nothing to do with the problem and won't fix it.

I successfully migrated all the media to D11:

After applying "3494209-14.patch", I ran into 🐛 Migrations fail due to missing dependency when dependency has skipped rows by the source plugin Needs review , because with the patch applied, the dependencies seem to not get resolved correctly anymore. So I manually removed the failing dependencies. After that, a MigrateException was thrown in `/src/EventSubscriber/MediaMigrationSubscriber.php`, line 256.

I simply added a return statement above and everything worked as expected:

    if (empty($final_source_field_name[0]['field_name'])) {
      return;
      throw new MigrateException(
        sprintf(
          "Cannot identify the the media entity's source field name"
        )
      );
    }

This is quite dirty, but everything seems to got migrated just fine! Of course we should find the real cause of this endeavour.

Just ran into this once again. After trying to migrate a site using media_migration enabled and a dirty workaround to get it to work with D11 ( 🐛 Drupal 7 to Drupal 11 migration runs forever Active , #14), I tried the latest 3 patches provided here, but nothing seemed to work.

I ended up simply manually removing the failing dependencies from the migration ymls. Luckily, they still migrated in the correct order, and the files migrated to media as expected. I think this is how I fixed the issue last time I ran into this.

I hope this might help someone.

Just a few phpcs fixes.

Dirty core patch for the time being.

@Anybody and I will have a deeper look tomorrow.

📌 Optimize SQL queries RTBC doesn't solve anything unfortunately.

Ok for some reason, the added array_map changes from https://git.drupalcode.org/project/drupal/-/merge_requests/1797/diffs?fi... in core migrate results in this issue. If we revert getMigrationDependencies() to how it was in D9: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/migr... drush migrate-import --group=my_migration_group --continue-on-failure won't be stuck in an endless loop.

I have no idea, how this module affects that line of code. I already commented out all code inside MigratePluginAlterer and in the module file. But it is still stuck in that endless loop, unless I comment out this modules MediaMigrationSubscriber "onPrepareRow" method.

Bug is back on the table...

Setting back to major, as the module isn't d11 compatible yet anyway.

But this can already get reviewed.