🇬🇧United Kingdom @AndyF

We'll want to add something to the release notes about changing the constructor signature.

Thanks, I was looking for just that! I've made some updates:

• Use create_record_uuid as the boolean and record_uuid_field as the field name for consistency with create_record_number and record_number_field.
• Don't force the ID to be the UUID. (I figured it's a little more flexible this way.)
• Add a section to the unit test for this.

I've updated \Drupal\migrate_source_csv\Plugin\migrate\source\CSV::__construct() in a non-BC way. Per Drupal BC policy I think it's not considered public, and a number of people recommend extending create() instead of construct(), so I thought it might be ok to keep it simpler and not add a deprecation.

Thanks!

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

AndyF → created an issue.

AndyF → created an issue.

Re #18 and it being a runtime check, some excerpts from the PHP internals discussion

Just to make sure there is no misunderstanding I'd like to clarify that the proposed check of this RFC is not a runtime error. It's a compile time error, just like the check for incompatible method signatures during inheritance. In fact it shares a non-trivial amount of the existing logic of that check, because it's so similar.

My understanding is that once the class is successfully sitting in Opcache there will not be any further overhead.
— source

and a response

Although OPcache is ubiquitous in production environments now, it's not obligated and the cache only lasts as long as the SAPI process.
— source

This may already be clear to everyone but it wasn't for me (:

Just to chime in re #12 I was seeing exactly the same thing (see https://github.com/ddev/ddev-selenium-standalone-chrome/issues/41). It appears that \WebDriver\WebDriver::session() is checking for the w3c flag one level higher up than where it's being set. I'm not sure if the issue should be fixed in lullabot/php-webdriver or by changing the capabilities we're passing. Locally I've just changed the capabilities, but I've seen plenty of examples on Stack Exchange etc. of the w3c flag being within goog:chromeOptions and I can't find a canonical reference on the matter.

The MR was automatically put into draft by a fixup commit (I guess if I'd rebased with autosquash it wouldn't have), and I don't seem to have permission to remove the draft status; I'm not sure it makes a meaningful difference to the state of review of the code contained within however.

There was one failed test that's a known intermittent, see 🐛 [random test failure] Random test fail in EntityReferenceWidgetTest Active . I've just started a rerun.

It looks like there's an unanswered question of ticket scope: are we only interested in external URLs, or should we also deal with internal? Personally I'd suggest that if we can't simply solve both, then it makes sense to at least support external URLs along the lines of #31 and if we want to handle internal links do that in a separate ticket. The only issue with #31 that I can see is the slight WTF you'd get from adding an entirely separate query param to such an external URL: I don't think you'd expect that to change how the duplicate query parameters are handled. ie...

// Assuming we're using a solution that only implements #31.
\Drupal\Core\Url::fromUri('https://example.com/page?tag=one&tag=two')->toString();
// Returns https://example.com/page?tag=one&tag=two
\Drupal\Core\Url::fromUri('https://example.com/page?tag=one&tag=two', ['query' => ['foo' => 'bar']])->toString();
// Returns https://example.com/page?tag=two&foo=bar

@carolpettirossi could you give any extra detail on why/how you're using local links with duplicate query parameter names? Thanks!

Duplicate: 🐛 Url only outputs the last value of a query parameter Needs work (Also related: ✨ Output multiple values of a query param with repeating param name Needs work )

Sure, then that leaves the question... dya wanna have no linting, or custom rules? (:

@doxigo In the 2.x branch I made minimal modifications to the Twig (just to remove the bits displaying normal local tasks) and didn't touch the CSS at all. Now we have CI set up, we have some style linting going on with lots of failures. I figured this is very much your wheelhouse, so leave it to you to decide if you want to update the CSS, update the rules, or just not lint the CSS at all.

Thanks!

AndyF → created an issue.

I tested the update from 1.x to 2.x and it successfully converted the config from the old format to the new.

Thanks everyone for the work so far, we also noticed our site caching being impacted.

The cache only needs to be invalidated once the module settings are changed.

Seems then that we ought to add a cache tag! See attached, thanks! (For 3.x)

We could possibly recognize entity routes when trying to select an icon, eg. entity.*.edit-form.

- isn't that what we are already doing? or what exactly you mean by recognize?

Yeah that wasn't very clear, and you're right, what we currently do is pretty much that. I noticed that we're dong substring matching eg. on edit_form but also on webform.edit_form, and I was wondering if we could match on a regex that detects an entity route as a whole, rather than a substring match. The only real advantage of this approach though is that you can then do a little bit of extra logic to determine if a canonical route is the edit form (eg. for a media entity). Ie if the route matches entity.*.canonical then check if the route also matches entity.*.edit_form, in which case it's an , otherwise assume it's a .

Super minor tbh.

I've removed support for D9. Also added an optional test of the previous minor version (ie it has to be run manually), and ran that to confirm it works with D10.1.

Still haven't tested the update hook.

I've updated the tests to cover the previous major Drupal version, and this has uncovered at the very least I shouldn't be using autowiring or we should drop support for Drupal 9.

@fjgarlin got back to me and said the change to get the tests working is all as it should be.

I appreciate this is a big ol' change, I feel I probably should've been a little more constrained in my changes. But on the plus side it has some tests (:

I was a bit surprised that I had to make the commit c26b46b8 to get the tests working on CI - I've asked about in Drupal Slack at https://drupal.slack.com/archives/C223PR743/p1712336082170709.

Although there is a hook_update_N() I haven't tested it (not even manually). I also haven't tested it with Drupal 9 yet.

Some other possible todos I noticed while doing this (I'm being a bit lazy by not making individual issues, but just wanted to get them down, and they're not necessarily that important).

• Add a functional JS test that verifies links are shown/hidden on click, and that they show up on the correct side.
• Could we add a default icon in case there isn't one? Or alternatively just not show a link in the sticky tasks?
• When not using the block, we could provide a textarea for users to specify paths, similar to block visibility
• We could possibly recognize entity routes when trying to select an icon, eg. entity.*.edit-form.
• Not my strength, but I think it's better to add classes than inline styling via JS, where possible.
• Also not my strength, but I was wondering if a transition would be a simpler way to achieve the animation than, er, an animation (:

Thanks!

To get this working on GitLab CI, which runs tests in a subdirectory, I used the following:

$this->assertSession()->linkByHrefExistsExact(Url::fromUserInput('/admin/structure/block')->toString());

AndyF → created an issue.

AndyF → created an issue.

Thanks, I've commented on the MR.

Thanks! Could you provide some details on what problems this was causing and why the change is more correct than the existing code?

Thanks! I think I spotted an issue, moving to NW.

It would be great if someone could update \Drupal\Tests\commerce_afterpay\FunctionalJavascript\ApiIntegrationTest to test the new functionality as well, I don't have any credentials at the mo.

Merging because it makes sense per the documentation and is clearly major, but leaving open for feedback on \Drupal\Tests\commerce_afterpay\FunctionalJavascript\ApiIntegrationTest.

Thanks @corniflex!

Rebased against 1.x and updated changelog.

I don't have access to a site with Afterpay anymore so wasn't able to run the API integration test. (It's not extensive anyway, but much better than nothing.) I've updated the test so that instead of adding one item to the cart, it adds two items which now cost half the price. Otherwise the test remains unchanged, so hopefully it'll work - but as I say, I can't run it without sandbox creds.

If some kind soul with access to a sandbox wants to run the tests, I'd very much appreciate it.

There's a functional javascript test that can be run to test the checkout and
payment capture/void process. The test assumes that Afterpay will accept a total
of ~45 USD.

1. Set up an account on the sandbox portal (eg. at
   https://portal.sandbox.afterpay.com/us). You'll need to set the password and
   it should already have a single saved payment method.
2. Set the following environment variables:
   • COMMERCE_AFTERPAY_TEST_MERCHANT_ID<\code>
   • COMMERCE_AFTERPAY_TEST_SECRET_KEY<\code>
   • COMMERCE_AFTERPAY_TEST_COUNTRY_CODE<\code>
   • COMMERCE_AFTERPAY_TEST_CUSTOMER_EMAIL<\code>
   • COMMERCE_AFTERPAY_TEST_CUSTOMER_PASSWORD<\code>

https://git.drupalcode.org/project/commerce_afterpay#testing-against-the-sandbox-api

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

Thanks everyone!

Duplicates 📌 Drupal 10 Compatibility RTBC .

Included fixes from 📌 Automated Drupal 10 compatibility fixes Needs review to get the tests working, rebased with 1.x to use GitLab CI and updated the changelog.

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

Looks like failures due to D10 compatibility which is addressed in 📌 Drupal 10 Compatibility RTBC .

AndyF → created an issue.

Thanks @Anybody! I'm happy to make a new ticket but thought I'd just gently push back once (: the cacheability metadata might explicitly be for the contents of #attached, so fixing only #attached might not give you correctly working attachments. Maybe it makes sense to deal with them both in one go?

Not sure if this belongs in a new ticket, but as far as I can tell it also doesn't propagate any attached cacheability metadata that's directly on the field element either, which surprised me given the explicit mention of cacheability on the project page. (I wonder if I'm missing something...)

Thanks for the detailed description @mnico. Moving to NW to catch #26 in a test.

Thanks for the feedback @mnico. Could you elaborate a little on how it's failing? Is it simply failing to make any difference at all to the behavior? Anything in the console? I tried locally using the test module against latest (c753ec180166fd9a12af9d9d047730d2ee2650b6) and the patch in #17 still seems to fix the issue where an ajax callback that doesn't trigger submit handlers can lead to data loss. Maybe there's another issue at play here?

I tried to rerun the tests on #17 but the patch didn't apply, here's a reroll.

Thanks!

Thanks @AstonVictor!

AndyF → created an issue.

AndyF → created an issue.

Rebased off 3.0.x, it no longer includes external code and is ready for review/merge. Thanks!

Rebased off 3.0.x, it no longer includes external code and is ready for review/merge. Thanks!

It would also be good to add a cacheablemetadata object to the events so they can influence the final render array cacheability... I was thinking it might make sense to wait for 🐛 [pp1] Improve cacheability metadata Postponed to land before adding that.

Thanks @kopeboy!

The MR already removes the section at the end of the module, though note if I understand correctly the same thing will happen anyway due to Drupal core. \Drupal\Core\Entity\EntityBase::invalidateTagsOnSave() calls ::getListCacheTagsToInvalidate() which adds the tags ENTITY_TYPE_list and if there's a bundle ENTITY_TYPE_list:BUNDLE.

I've pushed a start on that. Please note if reviewing that it includes work from 📌 Re-implement PrevnextService::getNodesOfType - performance issue Needs review and any comments on that code belong in that issue.

Thanks!

AndyF → created an issue.

I've added a first stab at that. If reviewing please be aware that some of the changes are from 📌 Refactor building links render array into prevnext.service Needs review and any comments on those changes should be made on that ticket.

Also I thought I'd explain the latest commit (fc936c4eb444158c9d6c08f9849c8d639f67a032). Without that commit I think the caching is correct, but it adds the config:prevnext.settings cache tag to every single entity view, which means every entity view gets purged on any prevnext config change, regardless of whether it's using prevnext. To avoid that I removed the cache tag and instead explicitly flush the entity bundle tags when the module config changes. It might be overkill, but I felt guilty just adding the config cache tag to every single entity view (:

Thanks!

AndyF → created an issue.

Here's a first attempt at that, feedback welcome, thanks!

AndyF → created an issue.

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

Re #6 Looking more closely I think that's at least partly due to the site using the contrib Plugin → module which has a plugin cache with a collection name that includes the hash of all enabled modules, meaning that as modules are (un)installed the cache keeps growing IIUC. I'm not sure how much memory that was actually taking up for us, but it could be part/all of the problem for some folk.

@yovince do you use the plugin module by any chance? If so you might see some savings by adding this to a custom module:

function MYMODULE_modules_installed($modules, $is_syncing) {
  if ($is_syncing) {
    \Drupal\Component\FileCache\FileCache::reset();
  }
}

Hmmmn...

Thanks @dww, just what I was looking for! I think we should also check that it has the method before calling it. Feels silly including an interdiff, feels incomplete without (:

AndyF → created an issue.

Thanks everyone for the work on this. I hate to push the status back to needs review, but I think there's some unnecessary complexity in the previous patch as the base class already has the required service.

Not sure it's worth an interdiff with such a small patch.

Thanks!

AndyF → created an issue.

See MR, thanks!

AndyF → created an issue.

We were suffering the same problem, I came to the same conclusion, thanks for the patch, works for me! A super minor stylistic point but it might be worth either adding the condition to a new line with its own comment like Skip fields with custom storage or perhaps just updating the existing comment. Thanks!

Not sure why this isn't showing as fixed...

smustgrave → credited AndyF → .

I think this must be a hangover from:

https://git.drupalcode.org/project/recurring_events/-/commit/6bdc2517395...

I think that's right: in that commit the new trait doesn't set the field as revisionable although the original was, this seems to be the cause of the mismatch.

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

A possible thread to pull on: I was busting a 650M limit when syncing config and uninstalling 9 modules. By adding the following it can now run with a limit of only 350M:

function MYMODULE_modules_uninstalled($modules, $is_syncing) {
  if ($is_syncing) {
    \Drupal\Component\FileCache\FileCache::reset();
  }
}

Thanks @whiz11. I think it probably makes sense to handle Facebook related issues in a separate issue. (Also I personally think it would be better as a suggested package: if I use the module for instagram I shouldn't need to have facebook/graph-sdk imho.)

Here's a patch that just enables cron.

Thanks!

This template is only going to be picked up when using the view route of the block. As a preview of what the block will look like if that helps

Thanks for humoring me @smustgrave, that wasn't clear to me when looking at the IS and description of the linked module.

Thanks for the work on this... I could be misrepresenting, but I believe there were concerns raised with taking this approach in ✨ Ability to display block_content entities independently, also outside of Blocks Postponed: needs info however (eg. #2704331-19: Ability to display block_content entities independently, also outside of Blocks → ). Do we have a response to that at all?

Do we still need the patch for 2.0.1+?

Thanks for the update, no I don't think we need this patch anymore.

Anybody using the patch should probably uninstall jquery ui draggable, and possibly jquery ui as well (if it was enabled by this patch) prior to upgrading.

Update link to Jason Grigsby post.

Update link to Jason Grigsby post.

Updated the original that I'd taken from https://www.drupal.org/docs/7/theming/working-with-javascript-and-jquery... → .

• Change Drupal.theme.prototype to just Drupal.theme.
• Use arrow functions and template literals.

AndyF → created an issue.

I think that we should also look at wrapping the translatable values in the defaultSettings function with $this->t().

I dunno (: I initially thought that it made sense to always have one language returned by a plugin's default settings and let the config translation system handle any conversions, but I'm really not sure. I looked around and found a few examples of translatable text not being run through t() but then came across paragraphs which does (and which I feel at this stage is probably quite well tested on the i18n front!). I've asked a Q about it in Drupal Slack #localize:

Hi, I'm looking into #3102324 🐛 Labels are not translated Closed: duplicate and had a question about translating plugin default settings. I know if you have any translatable plugin settings and the settings come from config, then the config itself needs to be declared translatable in its schema (eg. using label). However it's not clear to me in that situation if the default settings should provide translated settings or not (ie should you run things through t()).
I was imagining the answer would be no, and found some examples that seem to back that up (eg. \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter::defaultSettings()) but then noticed that paragraphs does. Can anyone help? Thanks!

does changing the type from "string" to "label" automatically make it translatable?

That's my understanding. If you look at core.data_types.schema.yml you can see how it's defined. Gabor's cheat sheet is super helpful and has a bit on translatability.

are we sure that we need to change [the trim_suffix] schema from "string" to "label"?

Apparently Chinese uses a different ellipsis so I guess it makes sense: ⋯⋯

Thanks!

Ah, possibly duplicated by 🐛 Labels are not translated Closed: duplicate which also considers the trim_suffix as something that needs to be translated.

I think as it's config you should set the schema appropriately, in this case I think you'd want to switch them from string to label.

Thanks @smustgrave, it'd be great to move this forwards! However there were (at least) a couple of reservations raised with the general idea IIUC:

block_content entities simply aren't designed to be displayed in another way, if you check the view builder you can also see that it explicitly works around render caching and has that disabled.

So the answer to this might simply be that this is on purpose, if you want to display something with views or references or ..., then don't use block_content but either a node type or custom entity type. I understand that not everybody will like that but I'm not sure I see an alternative for keeping it working correctly and efficiently (=no double-render caching of the same content, for example) also within a block.

https://www.drupal.org/project/drupal/issues/2704331#comment-11863202 ✨ Ability to display block_content entities independently, also outside of Blocks Postponed: needs info

block_content entities are exposed as block plugins and saving reusable blocks triggers a block plugin discovery. The more of them you have the more expensive that becomes.

https://www.drupal.org/project/drupal/issues/2704331#comment-14604615 ✨ Ability to display block_content entities independently, also outside of Blocks Postponed: needs info

I don't really grok the double render-caching issue and it's not clear to me why using a custom entity type might be better

If I build a custom entity type that's similar to block content entities and is for embedded content... I can see at some point being asked to write a plugin to make it accessible to the block system so that we can place these embeddable entities that way too. And then aren't we back in the same situation?

https://www.drupal.org/project/drupal/issues/2704331#comment-13474270 ✨ Ability to display block_content entities independently, also outside of Blocks Postponed: needs info

Attaching a patch for composer from 6bfb4d583fbe3684cd73abb73ea995a414ee1b80, thanks!

Btw just in case you hadn't seen, but there seems to be a layout-builder related failure with D10.1 which I think is entirely unrelated to this MR, see eg https://www.drupal.org/pift-ci-job/2692049 → ... had me well confused yesterday wondering what I'd broken ;)

Thanks so much, glad to hear this could make its way into the module! Agree that tests are needed, and I'll get on that over the next few days.

Attaching a patch for composer from 8eab48f6e2443ecfd899a61e93f26c700940c2b1, thanks!

I've made a first attempt at that. It could be cleaned up a little and the testing increased, but I was hoping to get some initial feedback on the overall approach before going further with the polish...?

Thanks!

AndyF → created an issue.

In case it helps, I cleaned things up by running

UPDATE {node_revision}
LEFT JOIN {users}
ON {node_revision}.revision_uid = {users}.uid
SET revision_uid = 0
WHERE uid IS NULL

Thanks!

I'm experiencing the same problem with I suspect the same cause: the revision user no longer exists.

step 3 delete the newly created user and choose the option to keep the content and make owner as Anonymous user

I tried everything I could to reproduce the errors, followed the steps you provided but still no errors were shown to me.

I don't think the issue presents when you use the UI (because node_user_cancel() will update the revision uid to 0). However if you just bluntly run drush ev '\Drupal\user\Entity\User::load(123)->delete();' using the UID of the author of the latest revision of a node, then you won't be able to update the node and save.

AndyF → created an issue.

AndyF → created an issue.