🇬🇧United Kingdom @AndyF

Fix the first example that was passing the table name instead of the alias to \Drupal\Core\Database\Query\SelectInterface::fields().

Thanks @moshe!

See https://www.drupal.org/project/dtt/issues/3436284 🐛 DTT can use different cached container than running site causing issues with testing Fixed for complications from this approach

Thanks, that's super helpful. I think I'm in the clear if I rebuild caches in the DTT context prior to running the very first test (which is something I think is useful anyway for running tests during development). Also the idea of detecting DTT can be used for simpler non-containery changes, like a simple config override.

I’m not sure about the header proposal.

Not a problem, I'm not sure I can make it work anyway :p

I think I'm going to look at using the user agent string and setting it outside of Drupal.

Thanks as always for your prompt help, appreciate your time.

Ah...

Behat\Mink\Exception\UnsupportedDriverActionException: Request headers manipulation is not supported by Behat\Mink\Driver\Selenium2Driver

Getting some gitlab gremlins, I tried to create an MR, it seemed to work, but didn't show up on d.o., and now when I do a compare on gitlab it shows me the option to view the existing MR which links to this 404 https://git.drupalcode.org/issue/dtt-3513161/-/merge_requests/20

But the code's there, it's just the MR that's proving reluctant.

andyf → created an issue.

Oh dang, I missed the original request, and I can't really maintain the module myself because the site that was using Afterpay moved away from Drupal and Afterpay, so I have no creds. (I reached out to them directly in case I could have one just for supporting the module, but no dice.)

There we go.

andyf → created an issue.

Like #14 I'm finding it only actually breaks when devel's enabled; otherwise it generates the same warning but things carry on functioning.

And in case it's not clear, it's the media library widget that stops working for me (ie you can't remove the image), not the entity browser.

For me, with devel enabled and set to the PHP warning get prepended to the JSON responses preventing them from being parsed, it seems to be fixed with https://gitlab.com/drupalspoons/devel/-/merge_requests/149.

I was also seeing this error on a node edit form with both a media browser and entity browser. The patch from #11 fixed it. I've moved it to an MR, still could do with tests.

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

Thanks for all the work on this. I was about to add an issue to squelch 403/404s from the watchdog log, and thought there might be something more general floating around!

I've taken the patch from #113 → , made an MR from it, rebased against latest, and nudged the tests back to green again.

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

Agree that tests are needed, and I'll get on that over the next few days.

🙄

Thanks @alexpott!

So if we change to true are we fine to commit?

I had an open question about the warning:

There's also still the outstanding question about if we want to have a config option with a warning and default it to enabled? (ie if we default to it enabled should we remove the warning? As I say, seems odd to me to have a warning with an option that's enabled by default.)

Just wanted to say I think I was bitten by this on a site that doesn't have the description display explicitly configured (in which case I understand it defaults to ). I've opened 🐛 Element description overridden by Bootstrap when using defaults Active . Thanks!

Let's see what testbot says, thanks!

andyf → created an issue.

Hi, I'm also seeing this with Drupal 10.2.10 and Gin 3.0.0-rc13, only when is enabled.

is someone chose to wait that long think that's on them

I mean it's a little over 2 months it's been released, so it feels to me a little harsh to characterize people who haven't had the budget/opportunity to update in such a way, but you're the maintainer.

There's still the other question about the warning, and personally I'd argue it'd be sensible to update the release notes for 2.0.0 to mention the breaking change.

I get that, but for users upgrading from 1.x to 2.x, this is a change of behavior. Sorry I feel like I'm making heavy work of explaining this.

Thanks @smustgrave. I don't want it to sound like I'm arguing for one side over the other, it's just so far it all your responses have ignored a group of users it seems to me.

the current behavior is on

I don't think that's an accurate statement. For users on 1.x it's not on. For users on 2.x it's on. The ideal solution would've been imho to default it to off on upgrade when it was originally introduced, but enable it by default (if that's what you want) for new installs. That way nobody gets a change in behavior. That cat's out of the bag now, so somebody's going to be disappointed it seems to me. There's no way we ensure that we don't change current behavior for anybody.

Again, I'm not arguing for one side over the other, just making it clear there's another side that isn't represented in your responses so far from my PoV. I can personally see arguments both ways:

• It's better not to change people's markup without them opting in, so turn it off. It's safer to let those who are happy with the markup being altered to explicitly opt in.
• It's ok to make changes like this in a major version upgrade - don't inconvenience users of 2.x that already have it switched on

If we go the second route, I think there should be a big warning on the release notes (and perhaps the project page?) about the automatic change in behavior. Currently it's a very inocuous item in the notes (:

There's also still the outstanding question about if we want to have a config option with a warning and default it to enabled? (ie if we default to it enabled should we remove the warning? As I say, seems odd to me to have a warning with an option that's enabled by default.)

Thanks again

Thanks all!

Most updates don't actually have a message so I think we can drop that.

I appreciate most updates don't have a message, but as explained in this case you're going to be upsetting some group of users necessarily (or have I missed something there?). If someone's going to be upset, and we can give them a message to explain, I'm not sure why we wouldn't? Does removing it bring any advantages?

There's still the outstanding question about if we want to have a config option with a warning and default it to enabled? And would we want it enabled by default for both new and upgrading users?

This change will break people's existing sites who don't have a problem with the change.

I think that's perhaps a strong use of break: it will switch off a feature to prevent orphans IIUC, which is arguably less of a break than adding extra markup without warning and the associated visual regressions. Certainly now that it's already been introduced in a released version, there's no way I can see to avoid potentially upsetting some group: either it's going to be switched off for you and some will want it on, or it's going to be switched on and some will want it off. (In both cases we can add a message to the update to try and help.)

We can also default to having it enabled on a fresh install and disabled on update.

If we go the route of defaulting to enabled (either for a fresh install or on update), do we also want to adjust the warning on the option? It seems odd to me have a warning with an option and to default to having it enabled?

Thanks

Thanks for testing it @hartsak.

I'm not sure if there would be any better wording for the setting title, but as a non-native English speaker I wasn't able to come up with better suggestions :)

I agree it's clunky, I'm a native speaker but words were failing me (:

Open to suggestions!

I've taken a stab at that. I opted to default it to off (I added a warning that it might affect styling, and it felt weird to then default it to enabled) but I appreciate folk might prefer the other way around (:

Thanks!

Thanks all. I'm also seeing this, here's an example where you can see the impact of an update.

Previously the link was in line and vertically centered.

andyf → created an issue.

Just for additional context, I think the author of htmx responded at https://news.ycombinator.com/item?id=41782080.

+1

having option values and labels would be necessary if we want to be able to make config forms based on a schema retrieved over REST which AFAIK is a goal.

Related: 🌱 Enhance config schema for richer default experiences Active

Ah sorry, I am going blind (: I see where the URL is used, but not the query arguments...

        $orig_url = Html::escape(\Drupal::request()->getBasePath() . \Drupal::request()->getPathInfo());
        $orig_domain = Html::escape(\Drupal::request()->getHost());

I noticed that the actual change made also adds the url.query_args cache context. I could be going blind, but as far as I can tell the block build doesn't vary based on the URL or its query arguments (and it makes the caching less effective).

Neato, thanks (: I assume we're ok using GitHub actions, which will use up minutes from the pharborist org IIUC.

I've also opened a ticket to get tests running on pharborist/pharborist, not sure if that issue queue will be checked or who might have the access to set something up.

Thanks @jcnventura! I've made a PR - I had been hanging on, I haven't actually tested the changes by upgrading a module, I've only ensured the tests pass. I was thinking it might make sense to create a script that would use ddev to fire up a D9 site, install some modules and upgrade them, then do the same again with a D10 site, and compare the output... but I haven't found time yet.

Lovely, thanks @christianadamski.

Hmmn, , TIL...

The suggestion in the last comment seemed to fix things for us. But coming back to it the next morning without an issue on production to fix, and actually I wonder why jQuery.extend() was ever being used... it seems to me it's just used to shallow clone an array, whereas I thought jQuery.extend() was all about pulling properties from objects. (But it can work because array elements are properties of objects, but semantically it's not what we want I don't think.)

I've updated it to just use normal array functions, I think it's a little easier to follow and is still resilient to modification of Array.prototype. Thanks!

Very specific situation, but I just had this occur. It looks like in my case it was because a dependency update caused an addition to Array.prototype which polluted the results of shallowCopy in removeMapMarkers() in geolocation-api.js. This is down to using jquery.extend():

properties inherited from the object's prototype will be copied over

That's in contrast to Object.assign() which might make more sense to use.

To clarify what I'm saying open up dev tools on a page with jQuery loaded and run:

jQuery.extend({}, []);
Array.prototype.customFn = () => 42;
jQuery.extend({}, []);
// The result of this second one won't be empty.

I've generally heard it's bad practice to modify a built-in prototype, but by the same token should we program defensively and use Object.extend()? (Which also means less jQuery, yay!)

Thanks @anybody. I'd be happy to write the code, but there's enough open issues that are more important, I'd rather wait for them to get merged (to avoid both stealing time from them, and possible conflicts).

Just to make the suggestion crystal clear (hopefully), the following would indicate that prev/next is enabled for the page and article bundle of the node entity type and the tags bundle of the taxonomy_term entity type.

prevnext_enabled_entity_bundles:
  node:
    page: page
    article: article
  taxonomy_term:
    tags: tags

Thanks

I think that's the entity ID, not the entity that's being added? I'm not sure the linked entity is loaded currently at all. Maybe it makes sense just to add the entity ID and entity type ID, and that can be acted on in a preprocess?

I'm not sure how actively it's maintained, but it's not abandoned. I'm using it on the one site, which is where the sudden activity came from. I'd be happy to be a maintainer at least while I'm supporting that site if that would help out the current maintainers, but very happy just to work in the issue queue.

I think the entity added by the current MR in ✨ Provide the entity (object) instead of the ID in the twig template for more flexibility Active is the entity the links are on, not the entity the links are pointing to, so I'm not sure this would work? (I haven't tested it, so possibly just missing something.)

Also it would be important to consider cacheability metadata: if using the linked entity's title, we'd want to add the linked entity as a cacheable dependency, so if the entity's title is updated, the stale cache item won't get used.

Thanks @tjmoyer. I'm seeing the same thing. I'm using the patch from 📌 Not compatible with Ckeditor5 Needs review (99f94562) with CKEditor 5 and it started occurring after upgrading from 10.2.7 to 10.3.0 (which includes an update of CKEditor from 41.2.0 to 41.3.1).

Are you also running D10.3+ with CKEditor 5 and the accompanying patch? If so, you might want to try the latest update to 📌 Not compatible with Ckeditor5 Needs review (and if it works for you, just close this issue).

I'm using this patch and found it stopped working for me after upgrading to 10.3 (even after using the patch from 🐛 linkit_media_library.opener.editor must be an instance of Drupal\media_library\MediaLibraryOpenerInterface Active ). I was seeing the symptoms described in 🐛 Inserting selected isn't seen by Linkit field as not empty Active : after selecting an item from the library, I couldn't submit from the link modal (the button's disabled), and if I clicked on the URL in the textfield it would just use a normal link with an href of /media/xxx (and no data attributes).

It seems to be resolved by changing

linkFormView.urlInputView.fieldView.element.value = attributes['href'];

to

linkFormView.urlInputView.fieldView.set('value', attributes.href);

which matches what linkit does afaict.

It would be great to get the tests running and passing on GitLab CI...

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

Tests of pharborist and drupalmoduleupgrader pass with 8.1 using the latest from the fork.

Just FYI I've forked pharborist and started working on updating it so the tests pass on higher versions of PHP. I've got it passing on 8.0, and using that the tests for this module also pass on 8.0 except for the existing failure mentioned in #20.

I'm going to look at getting it working with 8.1 now.

I've set up GitLab CI. I was hoping to have a matrix where we could test D9 with PHP 8.1, but that didn't work, so we're now just testing:

• D9/PHP 7.4
• D10/PHP 8.1

D9/PHP 7.4 is already failing without changes from this branch, see 📌 Use GitLab CI and fix tests Needs review . I'm not sure how best to continue testing D9 when the template variables are adjusted and the current stable becomes 11.

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

I've added the possible solution suggested in the previous comment of ensuring we always return a node collection; as mentioned before I don't know how to test it properly with the module and I'm new to it, so careful review appreciated, thanks!

I don't know the module or pharborist at all, but I'm expecting to have a D7 upgrade project soon and thought I'd try and help push things forward a little.

The test error seems to be legit, I get it locally.

1) Drupal\Tests\drupalmoduleupgrader\Unit\Plugin\DMU\Fixer\DeleteTest::test
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 '<?php\n
 \n
-'
+function foo_permission() {\n
+  return array(\n
+    'bazify' => array(\n
+      'title' => 'Do snazzy bazzy things',\n
+    ),\n
+  );\n
+}'
/builds/issue/drupalmoduleupgrader-3465500/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
/builds/issue/drupalmoduleupgrader-3465500/tests/src/Unit/Plugin/DMU/Fixer/DeleteTest.php:50

As far as I can tell it's down to \Drupal\drupalmoduleupgrader\Plugin\DMU\Fixer\Delete::execute() calling \Drupal\drupalmoduleupgrader\Plugin\DMU\Fixer\NodeCollectorTrait::getObjects() and getting back a \Pharborist\Node rather than a \Pharborist\NodeCollection. That in turn seems to be down to \Drupal\drupalmoduleupgrader\IndexerInterface::get() returning a NodeCollection with some indexers and an individual Node with others (at least with \Drupal\drupalmoduleupgrader\Plugin\DMU\Indexer\Functions::get()).

All usages of getObjects() immediately iterate the result, so it feels like it could be corrected by ensuring we return a NodeCollection. However I don't see how it was ever working, I don't see any obvious changes in the affected code that caused it to start failing, and if I revert the module back to 1.9 (and use the version of jcnventura/pharborist from the same date) I still get the same failure, so I'm nervous I'm missing something here. Also annoyingly I don't know how to get the module to use the delete plugin outside of a test, so I can't do a manual functional test and verify there definitely is an issue with the delete plugin, and it's fixed with the change. I don't see any issues in the queue that might be related.

AndyF → created an issue.

There's an alternative approach in 📌 Move field migration logic out of drush command Needs review which doesn't require using drush to trigger smart date migrate functionality from code.

Thanks!

Here's a first pass at that, thanks!

AndyF → created an issue.

Just to echo the first part of #55, if you install vanilla Drupal with bootstrap styles 1.1.5 and then update to 1.1.6 you will end up with an error message because you didn't uninstall media library theme reset before it got removed from the codebase. I think this needs a really clear warning in the release notes.

Re the last part of #57 I don't think it can be solved in code (eg. in an update) because it's plausible a site still wants the module enabled (it might even be a hard dependency of some other module on the site).

Thanks @sokru! I'm using 3.x, and my config has

prevnext_enabled_entity_types:
  node: node
prevnext_enabled_entity_bundles:
  node:
    page: page

There's no need for prevnext_enabled_entity_types because node is already present within prevnext_enabled_entity_bundles if you see what I mean.

Thanks!

Gah, forgot about that pesky fixup commit when I rebased, so it's now marked as draft again, sorry! (But I did rebase it a second time and fixed up the fixup commit, so it should be the last time we have the problem.)

Link to module

I've updated the comments based on the most recent feedback.

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.