Account created on 13 October 2005, over 19 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom catch

Project browser and automatic updates are enabled out of the box, and at least automatic updates depends on update status. Project browser at a minimum needs to make http requests to d.o even if it doesn't rely on automatic updates (not sure about that bit). Drupal CMS doesn't really exist without project browser because otherwise you can't install recipes and other modules etc., it may end up getting integrated into the installer itself.

So I think this would probably need to be a required checkbox (e.g. you have to click it to continue through the installer), which I normally would not like, but we could include a sentence that this can be changed after install (e.g. you can uninstall those three modules).

🇬🇧United Kingdom catch

I would imagine this would be conceptually similar to the old “web of trust” system where a person (ideally local) who knows and collaborates with a person over a period of time vets them out reducing the burden and limiting the travel needs.

I can think of one long-term and fairly prolific Drupal core contributor who is in Nigeria and doesn't list any Drupal events on their d.o profile. I can't off the top of my head think of any other Drupal contributors in Nigeria - there will be some, just don't know any by name off the top of my head. It would be at least very challenging to arrange an in person verification.

On the other hand they have a very identifiable contribution record over a long time - infrequent but very useful contributions to tricky issues usually. Otherwise I would not have remembered them enough to be able to look up their profile and check the details for this discussion.

A bigger issue is that long term and prolific contribution to core doesn't help you get the git vetted role, but that's 🌱 More flexible language for git vetted status for co-maintainers of existing projects Active , not this issue.

🇬🇧United Kingdom catch

Updated the issue summary to be a bit more generic, I think that shows that both examples can be valid.

🇬🇧United Kingdom catch

Ill note it shows we finally got vendors to adopt standardized training processes with formal material.

No, it doesn't show that at all. The only reason to apply for co-maintainership of a project is because you use or need it, often with a history of working on issues. That is not the case here, not even the pretence.

🇬🇧United Kingdom catch

@penyaskito I think the first case could be simplified to 'any release of any module that removes a composer dependency on another Drupal project'. Not at computer but can update later.

🇬🇧United Kingdom catch

It runs on the testbot, but at a huge cost, 36 minutes

OMG definitely not, let's remove the test from the MR.

Could the max_input_vars error get hidden by a subsequent error?

That seems possible, but I don't think we know exactly when it will get triggered, so we would need to hard-code support for it in core's error handler probably - maybe that would be OK though.

🇬🇧United Kingdom catch

Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

🇬🇧United Kingdom catch

Quick fix is fine but also we should use 📌 Add PerformanceTestTrait::assertMetrics() so it is easier to write performance tests Active in this test - it was the one performance test that didn't get converted in that issue.

🇬🇧United Kingdom catch

📌 Provide a Workflow FieldType that references a workflow entity and tracks the current state of an entity Needs work might help with the content moderation follow-up (or a follow-up after both of those issues land).

🇬🇧United Kingdom catch

A sample:

Hello Team,

Currently, we have good team for managing this module.

Also, we are working on Image Widget Crop of Drupal 10 to Image Widget Crop of Drupal 11 project. In future, we will plan to upgrade in Drupal 12.

Can you please grant me access of co-maintainer so we will do good work in this module.

Hello Team,
We currently have a strong team managing this module. Additionally, we are working on upgrading verf from Drupal 10 to Drupal 11, and we plan to upgrade to Drupal 12 in the future.
Could you please grant me co-maintainer access so we can continue to do good work on this module?

Hello Team,
We currently have a strong team managing this module. Additionally, we are working on upgrading image_url_formatter from Drupal 10 to Drupal 11, and we plan to upgrade to Drupal 12 in the future.
Could you please grant me co-maintainer access so we can continue to do good work on this module?

This is pure copypasta and I think it should be treated as spam.

🇬🇧United Kingdom catch

We should check whether the current logic works with taxonomy/term/n pages which are overridden by views by default. I have a feeling that it won't because of Upcast named arguments/named parameters in views Needs work , but also that it might just start working once that issue is fixed. Either way not commit blocking here but good to verify what happens and maybe open a follow-up to track it.

Changed is a state we would use when the page has been published but there's a forward revision for the entity. This way it becomes clear that what the user is seeing, is not actually what is visible in the published site.

I might be missing something, but I'm not sure this is a problem with content_moderation as such.

Wouldn't this only happen if the user is reviewing the actual revision page? Other entity pages should only show the default revision anyway so it will either be published or not, and it'll accurately reflect what they see.

Showing the workflow state is different - that also applies to unpublished entities in a draft state, even if they're also the default revision. So it's about something more granular than 'unpublished' then. On the edit form, they'll see the draft, but that's shown clearly on the edit form.

Workspaces will behave differently but we already have the workspace indicator, so users theoretically should already have an indication they're seeing things as they are in the workspace.

If the user is viewing the actual revision page, then they ought to know they're on it, although it could still be confusing if there's a mis-match.

If the above is right, I think we could special case the 'revision' entity link route (either here or in a follow-up), and then handle customisation in a further issue? But it doesn't seem like an issue with forward revisions specifically to me but more workflow state indication regardless of whether dealing with a forward revision or not.

🇬🇧United Kingdom catch

Discussed with @thejimbirch in slack and decided to move this to core. If there's a really good reason to keep it against the recipes and distributions project that's fine but seems more historical that it was posted there - now that recipes is in core and we've made several changes directly.

🇬🇧United Kingdom catch

I don't really know what the process is for these kinds of pages (which feels like a general problem and why we have such a huge quantity of under-maintained documentation), I would assume either marking them deprecated or unpublishing. My preference would be for unpublishing the obviously outdated pages and adding redirects for those paths to a canonical page.

I don't think we should ever actually delete because that's irreversible unless it's spam or something.

I vaguely remember there was an archive section in the handbook, but that might not be the right name.

🇬🇧United Kingdom catch

Adding some rough ideas on how to collect data to the issue summary

🇬🇧United Kingdom catch

One minor nit on the MR otherwise this looks great! Hopefully there's an existing test that almost tests this which we can extend.

🇬🇧United Kingdom catch

An MR with test coverage would be good.

The solution in #5 sounds preferable if that works.

🇬🇧United Kingdom catch

Tagging as a Drupal CMS release target. If we were to implement this in update module, would that save an additional opt-in? It's almost exactly the same data that we already send from update module so might be fine. Then update module needs to do something with the event - would suggest adding it to a queue item, and then having the queue runner actually send the data so it's as light as possible when applying a recipe.

🇬🇧United Kingdom catch

If it works on gitlab, could we check max_input_vars first, and if it's over a certain number, skip the test? That would mean the test doesn't time out locally and give people a hint how they can run it locally if it's likely to fail.

🇬🇧United Kingdom catch

Yeah that sounds good to me.

🇬🇧United Kingdom catch

Haven't reviewed the MR yet, but now wondering if we might need something like this for hook discovery too, e.g. if a hook class uses a trait from a module that's not installed or similar?

🇬🇧United Kingdom catch

Replying to the bc question from... this time last year.

The main case for block titles being altered would be previewing forward revisions, but workspaces will handle that when you're in a workspace via entity query rewrites.

For trash module I think that it will rewrite entity queries to not include the entity when it's trashed - but we should ideally check with @amateescu.

I think this is a sufficiently serious issue that we should not put the fix behind a feature flag but would be good to confirm the trash case first.

🇬🇧United Kingdom catch

We discussed this in slack - the 'soft' deprecation so that people don't jump the gun here is good.

I also wondered about a 'hard' (normal) deprecation in a future release so that we eventually tell people to remove it, but the risk of people still jumping the gun (e.g. breaking support with Drupal < 11.2 prematurely) will be there, and then when we finally get to a core release where that's not a problem, we might as well have just removed the supporting code anyway instead of formally deprecating the hook. That means some modules could still ship with crufty hook_hook_info() implementations, but when we eventually deprecate .module files altogether, it will get deleted with those.

🇬🇧United Kingdom catch

@flyke yes we should add validation for the library name, maybe via an assert() so it impacts developers rather than live sites, but could also log on top of that.

🇬🇧United Kingdom catch

Yes we might be able to delete this code altogether after 📌 Use LRU Cache for static entity cache Postponed .

🇬🇧United Kingdom catch

Don't think this makes sense, the contents of 404 pages are usually pretty cacheable, unless you're using search_404 or something but that can set its own max-age.

🇬🇧United Kingdom catch

This all changed in the meantime, we have policy exceptions for entities with base classes, the 1-1 rule for one-class interfaces etc.

🇬🇧United Kingdom catch

This was mainly for Drupal.org which handles cross-posting a lot better, and will soon use gitlab for issues anyway.

🇬🇧United Kingdom catch

Committed/pushed to 11.1.x and cherry-picked to 10.5.x and 10.4.x, thanks!

🇬🇧United Kingdom catch

Not validating also causes problems with the entity changed constraint protection against cross-updates, closed a very old issue of mine as duplicate.

🇬🇧United Kingdom catch

I think this is probably a duplicate of 🌱 Make pre-save entity validation more commonplace (e.g., the default) Active now - the only way we can enforce this is on validation.

🇬🇧United Kingdom catch

Someone in slack was reporting problems with this - cache tables either getting huge, but setting a limit leading to constant deletes. So probably still relevant.

🇬🇧United Kingdom catch

Duplicate of 📌 Add auto-refresh/quasi-LRU to database backend Active (yes I opened both within a couple of weeks of each other).

🇬🇧United Kingdom catch

📌 Render the navigation toolbar in a placeholder Active is one part of this.

📌 Entity lazy multiple front loading Active is another.

🌱 [meta] Placeholder-driven performance improvements Active is yet another.

Given those three issues exist, closing this out.

🇬🇧United Kingdom catch

This is still relevant, looks like the code hasn't changed.

🇬🇧United Kingdom catch

At least with bigpipe this is no longer special cased, see 📌 Use MessagesCommand in BigPipe to remove special casing of the messages placeholder Fixed . I also haven't noticed lots of rendering time when profiling.

🇬🇧United Kingdom catch

This has all changed in the meantime, closing out.

🇬🇧United Kingdom catch

JSON support is in progress and we're replacing serialized php with that mostly.

🇬🇧United Kingdom catch

#2506369: Cache CSS/JS asset resolving didn't remove all the calls, but LibraryDependencyResolver has static caching now.

🇬🇧United Kingdom catch

Still valid unfortunately.

🇬🇧United Kingdom catch

I think this is probably still valid, linking some related issues. We should re-check how many update queries get run by the router/menu rebuild.

🇬🇧United Kingdom catch

#18 is wrong, we do router rebuilds immediately when installing modules. See 📌 Review Route rebuild in ModuleInstaller Active .

🇬🇧United Kingdom catch

We can just keep reducing the number of database queries required to serve a page, in issues like 📌 Entity lazy multiple front loading Active .

🇬🇧United Kingdom catch

Include files are nearly gone.

🇬🇧United Kingdom catch

A session will be created in CsrfTokenGenerator::get() if one doesn't already exist now.

🇬🇧United Kingdom catch

This is amazingly still relevant, but won't be after 📌 Handle module preprocess functions Active so closing out. We can look at attribute parsing improvements or changing how the theme system works in general.

🇬🇧United Kingdom catch

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Let's add the test coverage here and then consider modifying it in the other issue if we need to.

Committed/pushed to 11.x and cherry-picked to 11.1.x, 10.5.x, 10.4.x, thanks!

🇬🇧United Kingdom catch

This stalled but we still need to figure something out once 📌 [meta] Convert all core plugin types to attribute discovery Postponed and[#3265945] are finished. It's looking like we might need to support plugin annotations until Drupal 13 to allow time for it to fully percolate through contrib plugin types and custom implementations

I'm wondering if we could do something per module, either a .info.yml file flag or a parameter as we did for module.hooks_converted = TRUE, that would allow files to be completely skipped for annotation parsing, this would be more reliable than a site-wide flag then. It would then also be possible to aggregate this and show in hook_requirements() whether it's safe to disable opcache.save_comments.

🇬🇧United Kingdom catch

Do we actually have a follow-up issue open for the UX changes? Would be good to open that and maybe attach the most relevant screenshots from here. Tagging for that.

Opened one more follow-up based on the placeholder discussion in the MR: 📌 Try to use a parameter for the workspaces toolbar destination link Active .

Can't review the CSS here usefully, and only just about bits of the JavaScript, but other people have done that, and the PHP looks fine.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

[PP-2] Support inline and async CSS Postponed: needs info is already open but there was no progress there and I marked it 'needs more info' some time ago, so possibly we should start fresh and mark that issue duplicate of this.

🇬🇧United Kingdom catch

Since this is preventing development on the 1.x branch, bumping to critical.

🇬🇧United Kingdom catch

We're just using 10.5.x/10.6.x and haven't missed a 10.x branch. Closing.

🇬🇧United Kingdom catch

Oh I didn't reread the issue summary after coming back here from slack. Let's use the text in the issue summary except something brought up in slack was thay n years y months will be extra work especially for a person, so we could replace that with 'several years'.

We should add a tag - best way to track re-opened issues.

I think with a bot we should rate limit it to 5/day to start with, but ramp it up to 10/day if it's going well. The main reason to rate limit was not to overwhelm people's issue trackers with bumped issues but that's less of a problem if a person is doing it.

For a manual trial by @smustgrave 10/day seems fine - any of us could triage an issue at any time.

On issue type.

I think it could be worth doing something like this:

Active tasks
Active features
Needs work tasks
Needs work features
Active bugs
Needs work bugs

This is on the basis that over the years we have definitely opened thousands of minor tasks to clean things up which then got forgotten about, but we're then fixed or made redundant by other issues in the meantime. It may be less true for features/ bugs so doing those in subsequent passes lets us iron out issues before we get to them.

🇬🇧United Kingdom catch

Adding API support is a good idea, we could try to implement it in Olivero too maybe.

Allow CSS to be added at end of page by rendering assets with placeholders Active is trying to defer CSS.

Are you adding this to individual files or the library definition? And what happens if a critical file has a dependency on a non-critical one?

🇬🇧United Kingdom catch

This looks good to me. I would generally agree with @berdir that it's better to do one change than two to the same code, especially if it leads to a double deprecation, but given the staleness of the issues this conflicts with, it shouldn't be too much churn.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Moving this to the core queue.

I have no idea how widespread disabling proc_open on shared hosting (or other hosting environments) is in 2025, but I think it would be useful to try to get data from at least a few major hosting providers.

Depending on that, and reports like the original one here from people trying to run automatic_updates/project_browser on hosting without proc_open support, we could then consider what to do.

It would probably require a fallback that bypasses Symfony's Process component altogether - but unless we re-implement the process component or swap out some classes, that probably means calling composer API functions directly instead of via the cli.

🇬🇧United Kingdom catch

Here's some possible draft text for a canned issue updated:

This issue hasn't been updated for more than eight years, we should confirm whether it is still relevant, and whether there is a more active issue addressing the same thing.

I am marking this 'Postponed (maintainer needs more info)' to begin with.

If you know that the issue is relevant, please move it back to active or needs work, and update the issue summary.

If there is a more recent issue, please close it as duplicate, linking the more active issue.

If there are no responses at all after three months, we will close this issue as 'outdated', however it will still be possible to re-open the issue after that time if it is re-discovered and found to be relevant.

🇬🇧United Kingdom catch

Yeah I'm not suggesting archiving that one, but I think it could live under the ddev instructions, for example - especially if it ends up the only item in the Windows section.

Most of the other stuff is dated, indeed. Just like "Installing Drupal with DDEV in WSL2 on Windows" will become ...

Any page will get outdated if no-one updates it, but at the moment we have somewhere in the region of 50 (complete guesstimate) pages with different instructions about setting up a local development environment, many of which are already 10 years out of date. If we can get down to handful of current pages, there is more chance of maintaining them in the future.

🇬🇧United Kingdom catch

@hansfn I mean at least most of it.

e.g. Windows has an entire XAMPP section which doesn't mention Windows 11 https://www.drupal.org/docs/develop/local-server-setup/windows-developme...

The bitnami Drupal stack page is already deprecated https://www.drupal.org/docs/develop/local-server-setup/windows-developme...

Also this - most of the comments about it in the past two years are about whether to remove it or not https://www.drupal.org/docs/develop/local-server-setup/windows-developme...

This also looks extremely outdated https://www.drupal.org/docs/develop/local-server-setup/docker-with-solr-...

🇬🇧United Kingdom catch

It's more of a revert of 📌 Rebuild routes immediately when modules are installed Fixed .

See the comments in ModuleInstaller:

if (!\Drupal::service('router.route_provider.lazy_builder')->hasRebuilt()) {
        // Rebuild routes after installing module. This is done here on top of
        // \Drupal\Core\Routing\RouteBuilder::destruct to not run into errors on
        // fastCGI which executes ::destruct() after the module installation
        // page was sent already.
        \Drupal::service('router.builder')->rebuild();
      }
      else {
        // Rebuild the router immediately if it is marked as needing a rebuild.
        // @todo Work this through a bit more. This fixes
        //   \Drupal\Tests\standard\Functional\StandardTest::testStandard()
        //   after separately out the optional configuration install.
        \Drupal::service('router.builder')->rebuildIfNeeded();
      }

🇬🇧United Kingdom catch

The MR is doing what I was asking for in #10 already - removing the #attached where it's not necessary. To me this is fine, custom form alters that somehow need form.js should also be attaching it.

Minor nit on the assertion comment but test looks good otherwise.

Long-term: Deprecation notice and remove the entire non-privacy-compliant function.

Let's open a follow-up for this, and maybe another one to audit form.js overall, it looks like there is potentially quite a lot of unused code in there.

🇬🇧United Kingdom catch

This is the core issue that originally added caching, there have been lots of changes since then though (because the original MR didn't actually work unless you were a core committer due to absolute paths), so easier to review the gitlab yaml in HEAD. 📌 Try to store the phpstan result cache as an artifact Active But as above we didn't try to cache anything that's a directory yet.

🇬🇧United Kingdom catch

Since we've missed multiple other gitlab backports between 11.x and 10.x, the effort of backporting everything now will probably be as much or more than dealing with any critical issues if they come up. So I'm going to just mark this fixed against 11.x.

🇬🇧United Kingdom catch

I don't think the MR will result in any caching between pipelines.

If the cache (or vendor, or both) dir is stored as an artifact, only from branch runs, and pulled down before composer commands are run in all pipelines, then composer update will deal with any staleness. Then the results of that are used for the other jobs. This might work but you need to manually pull down the artifact from the gitlab API.

We are using this pattern for phpstan et al caches in core, but it was very hard to get right and also those are individual files and not a directory. For example a lot of these caches are based on absolute directory paths and they are different every build. Pretty sure ci_project_dir is not the same for every MR. And build dirs are different depending on whether you have commit access to the repo or not, even for different pipelines on the same MR.

🇬🇧United Kingdom catch

Both the provider and the prompts are in the issue summary, so I'm not sure what #3 is talking about.

🇬🇧United Kingdom catch

Getting close on phpstan and phpcs but not there yet.

I think we might be testing against the latest tag of the lms module, so may need a new release there. Or at least, I'm getting different issues running locally to the ones on gitlab now.

🇬🇧United Kingdom catch

@TR, no when #12 was written, while there was no way to deprecate constants, we would remove constants that were unused by contrib in minor releases (probably), or if not, in a major release - in both cases with a change record since that was the only way to communicate the change.

It looks like @danielveza created the change record the same day, but forgot to come back and update this issue. Then also no-one else noticed or updated this issue in the intervening two years.

Had the issue been re-RTBCed, it could have been committed to either a major or minor release at that point.

During that time, we added better support for deprecating constants (not trivial because it can't be done at runtime, so it had to be added to phpstan), so that we don't break contrib modules relying on them, like the ones that Ghost of Drupal Past found in #9, and this can happen in any minor release now, no need to grep contrib etc.

I've now spent five minutes writing this up which I could have spent committing this issue if #25 hadn't been inaccurate.

🇬🇧United Kingdom catch

@tonypaulbarker yes I think that bit would need to come from #3497545-9: Reduce the number of display modes available on embedded images or something like it, but it looks like the responsive image configuration would need to be updated to support that.

🇬🇧United Kingdom catch

No it's a different error, the class needs to use CourseControllerTrait which does define this method. Might want to rename that trait if it's not only used for controllers.

🇬🇧United Kingdom catch

AnswerForm calls ::handleError() but it's not defined anywhere.

🇬🇧United Kingdom catch
  ├ The update failed with the following message: "Failed: Drupal\Core\Database\SchemaException: MySQL needs the 'alias' field specification in order to normalize the 'alias' index in Drupal\mysql\Driver\Database\mysql\Schema->getNormalizedIndexes() (line 332 of /builds/issue/drupal-3159210/core/modules/mysql/src/Driver/Database/mysql/Schema.php)."

To me this looks broken in the MySQL driver.

::addField() calls ::createKeysSql(), ::createKeysSql() calls ::getNormalizedIndexes(), ::getNormalizedIndexes() expects a $spec['fields'] to be passed in with the field definition(s) and that is not done.

Production build 0.71.5 2024