🇬🇧United Kingdom @AndyF

Closed in favor of 📌 Run tests on CI Active .

andyf → created an issue.

Closing because this already has an issue fork associated with it, and I don't know if I'm gonna bork things by moving it to a new project and associating a second issue fork for a different project.

Appreciate this is a little late (: There's some initial code up, but no documentation and a need for more tests.

Thanks @avpaderno.

Posting a comment per the rules to show 14 days have passed.

Possible to get a test case showing this as an issue? Is it possible without field_groups/should field groups be the one to fix it?

I just recreated the issue on 11.x and it seems to be fixed by the PR from 🐛 TableDrag JS :first-of-type issues Needs work (which includes tests).

Also updating the repro instructions: as far as I can tell you need to drag the item up for it to fail.

Should this be marked duplicate?

Not really sure if active or NW is better when there's only a test in the MR, but moving it back to NW to match previous status. Thanks!

Doesn't seem to have fixed itself.

andyf → created an issue.

andyf → created an issue.

andyf → created an issue.

all loggers gathered at this point can be handled at once by adding an implementation of CompilerPassInterface that will run before the TaggedHandlersPass and update all services tagger with logger to wrap them with a filtering decorator before they are collected by the TaggedHandlersPass.

I started looking at this, and while I kinda hate to push this back further when what's there is eminently usable, I was thinking it might make sense to add a more general purpose collected_service_decorator tag to simplify adding decorators to any group of collected services. Don't want to add complexity, was just thinking we'd be pretty much writing it anyway, so it might make sense to do it in a general purpose way.

If that makes sense, I'm not sure if it should be a new issue where it can be worked on in isolation, or part of this issue where it can be used when it's introduced.

Thanks!

Update the service tag API link not to include a specific major version.

andyf → created an issue.

Thanks @shuklina! Moving to fixed as it's merged.

I'm moving this to the project ownership queue after 7 days, as there was no response from contacting @chaunceyt or @scottalan.

I think what some folk might be experiencing is connected with taxonomy terms becoming moderatable in 10.3 → . If you install this module prior to 10.3, and then update to 10.3, I think the module suddenly changes the base fields it's adding to taxonomy terms, without going through the normal process of installing the new fields. (So I'd assume the issue wasn't that developers had been forgetting to update properly, but rather they had already run the DB updates to add fields before they updated Drupal to 10.3.) Just a theory!

Thanks, I noticed this warning after removing openy_rose:

User warning: The following theme is missing from the file system: openy_rose in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of /var/www/html/web/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php)

I've simply removed lingering references, thanks!

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

Forgive me if I've missed something more recent, but just in response to this from #3410487-11: "Drupal\migrate_tools\Drush\MigrateToolsCommands" not found PathRedirectImportCommands → :

It extends MigrateToolsCommands because makes use of several methods declared in that class.

Extending DrushCommands would trigger other errors.

While that's true as things stand, I think there's room for improvement on the current approach. I've opened 📌 Don't extend MigrateToolsCommands Active to discuss. I think the current approach can't protect users from the risk of breaking when migrate tools is updated.

Thanks!

andyf → created an issue.

@swentel if you're able to give a bit of time over to a review of 🐛 TableDrag JS :first-of-type issues Needs work I could summon the courage to get back in on the dev side. I backed off a bit because it was getting at best cursory reviews, but it'd be neat to take it over the finish line... assuming tabledrag hasn't completely changed in the meantime at least (: Completely understand if you don't have time btw, jus thought I'd mention it.

andyf → created an issue.

Looks good to me, +1, thanks for testing it properly!

andyf → created an issue.

I'm happy to try and take a look Thursday or Friday, but can't promise :)

@rduterte I notice this is still assigned to you and don't want to step on any toes... are you or would you like to be working on it?

Thanks everyone! I took a look at the code and I think we can work more closely with existing core logic. The request path condition implements \Drupal\Core\Plugin\PluginFormInterface so we can use ::buildConfigurationForm, ::validateConfigurationForm, etc. You can see an example in BlockForm. Also the request path's schema is already defined as condition.plugin.request_path (without negate).

This would also make it easier if we wanted to expand to other conditions, eg roles.

Hey, I think there's an issue with the hook_update_N() that was committed. From the docs, it's safe to use

\Drupal::configFactory()->getEditable() and \Drupal::config(), as long as you make sure that your update data matches the schema, and you use the $has_trusted_data argument in the save operation.

(my emphasis)

Thanks!

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

Looks like this is fixed.

Looks like this was fixed...

Thanks all! Rebased against 211722d90c2fdedb17308f4e7999275c2b3df495.

I tested it out locally on a site with some large revisions (max 5MB) but is otherwise simple: one language, only the (single-value) body field gets large, < 20k nodes. I saw the dump size reduce to ~50% of its original size (uncompressed), and the MySQL (5.7) DB data directory fall to about 68% of the size, measured immediately after doing a fresh DB import.

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

Re #69

Perhaps we should look to implement this for all revision tables...

I was curious about InnoDB transparent page compression and read up a little, and noted a couple of points that might help inform whether to use it by default. From Percona articles around 2018:

1. It uses sparse files, which can have a large negative impact (eg. 9s to 52 minutes!) on file copying speed (eg. backing up by copying the data files directly) on spinning rust. It's also more finickity: "preserving page compression when moving a page-compressed tablespace file from one host to another requires a utility that preserves sparse files".
2. The added random operations increase the importance of using SSD/Flash based storage.

andyf → created an issue. See original summary → .

andyf → created an issue.

It seems to me there's a lot of overlap between this and ✨ Add way to "intern" large field item values to reduce database size by 10x to 100x for sites with many entity revisions and/or languages Active , although there's the mention of optimizing for inline blocks in this ticket.

An even further optimization would be to remove inline custom blocks' block_revision_id from the section data before generating the hash, and to have a separate database column containing those block revision ID values. That way, for the case where you edit the layout and the only edit made is to edit the fields of an inline custom block, then the new revision would generate the same section storage hash.

Is it worth postponing on that and leaving this as a potential optimization?

I added a top-level <code>description</code> property which seems to be allowed per https://git.drupalcode.org/project/drupal/-/raw/10.1.x/core/modules/sdc/....

Understood, thanks for humoring and learnin me (:

I've added aliases for everything with an interface, happy to go further if that's desired, thanks!

andyf → created an issue.

Although just idly thinking out loud, if there's no anticipation of swapping out services via the container, does it make sense not to have them as services at all and just use the class resolver? Not sure you win much, feels a bit like navel-gazing, but popped into my head (:

Beautiful, thanks for the confirmation!

The primary use of interfaces is defining an abstraction, so the services can be easily swapped with different implementation.

Right you are (: I think I misread the lack of interfaces as a signal that they might not be public, no point bloating things unnecessarily, thanks again!

andyf → created an issue.

andyf → changed the visibility of the branch 1.x to hidden.

andyf → created an issue.

Thanks everyone for the work on this.

If we lay hands on this, i'd say let's add interfaces for the factories too.

That's actually what I was looking for. Kinda minor, but on a site with Domain Access I'd like to make the Views UI domain-aware and was hoping to decorate the shared temp store factory, but I can't do that because there's no interface. (No great shakes, I can extend and override, but still...)

I've moved #26 to an MR and added factory interfaces.

• I didn't add type declarations to the new interfaces because of the existing implementations. I don't think these classes are covered by the BC policy, but I didn't want to make life hard for anyone extending them nonetheless;
• I didn't update existing type declarations that reference the concrete temp store implementations, again for abundance-of-caution BC reasons.

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

At the mo I've just rebased existing work and am currently taking a look through.

I've noticed what I think is a problem with the current approach: it locks the feed in the first item and unlocks it in the final item, but as they're cron queue worker items, they might not all get processed in a single run. So I think with sufficiently large workloads this would lead to feeds being locked between cron runs.

So it should get triggered on cron runs by itself. Do you think that makes sense?

Yup!

Is that doable without loading all feed types on each cron run?

It's been many years since I last looked at the codebase and I'm just getting acquainted with things so bear with me. IIUC we need to load the feed to get expired IDs, so the only way I can think of to avoid loading all the feeds on every run would be to add a new base field to track the last time it had a cron clear executed on it.

So a rough outline might be

1. Add a new timestamp Feed base field eg. cleared and set it to 0 for existing feeds.
2. On cron
   1. we execute a query to find up to entity_update_batch_size feeds that haven't been cleared recently (as indicated by cleared). For each of those feeds, if they have any expired IDs, we add the IDs to a per-feed expiration queue;
   2. the feed expiration cron queue worker clears any items in its queue.
3. Add a new processor setting for how frequently to check for expired items?

Question: in an approach like this, do you think it's ok to only get a lock when grabbing the expired IDs? Ie actually deleting the items associated with those IDs will happen by processing a queue and the feed won't be locked by the queue workers?

Thanks!

Rebased against ea64d3b8118d288d77e84f03e8cb1f70f45046f0.

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

andyf → created an issue.

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.