UK
Account created on 22 February 2008, about 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom longwave UK

Thank you for getting back to us, glad we could get to the bottom of it.

πŸ‡¬πŸ‡§United Kingdom longwave UK

> if we could widen Drupal 10's phpunit constraint to allow phpunit 10

I just don't think this will be feasible. The scope of the rewrites in PHPUnit 10 is just too large to try and support both at once. However, most tests should work just fine in both versions, unless they use some deprecated features - but even in those cases the fixes are backportable if needed.

> The big issue with phpunit 11, is will it be dropping support for phpunit 9 features that were only added in phpunit 10?

PHPUnit 11.0 is already out. PHPUnit works by soft-deprecating in one version, hard-deprecating in the next, and removing in the one after, so anything soft-deprecated in 9 will be removed in 11, but we should already have dealt with most of those as we have been on PHPUnit 9 for a long time now. https://phpunit.de/announcements/phpunit-11.html has a list of the breaking changes and some things are going away in PHPUnit 12, but we don't have to worry about that for a while at least.

PHPUnit 8 and 9 will continue to receive PHP version upgrades as long as their team decide it is feasible, so we can only hope that it will last for the lifetime of Drupal 10, but there are no guarantees. Symfony 6 will presumably have the same issue, as we share the same lifecycle.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Responded to the feedback.

πŸ‡¬πŸ‡§United Kingdom longwave UK

I'm using this with the paginator pager type and while it works well, it is repeating HTTP requests - and my source is relatively slow to respond, so this is slowing down my migrations.

If I add logging to Http::getResponse() I see that requests are made up to three times for the same URL:

get https://[redacted]?size=500... done
get https://[redacted]?size=500&offset=500... done
get https://[redacted]?size=500... done
get https://[redacted]?size=500&offset=500... done
get https://[redacted]?size=500&offset=1000... done
get https://[redacted]?size=500&offset=500... done
get https://[redacted]?size=500&offset=1000... done
get https://[redacted]?size=500&offset=1500... done
get https://[redacted]?size=500&offset=1000... done

I hate to say this at this stage but I wonder if the different pagers need to be their own type of plugin? That would mean they could be more easily extended for different cases.

πŸ‡¬πŸ‡§United Kingdom longwave UK

To backport this to 10.3.x I am not sure we can add the return types, we might have to add them to the baseline or ignore them instead.

πŸ‡¬πŸ‡§United Kingdom longwave UK

+1, Kim is a prolific contributor to core and contrib projects for a number of years and always provides high quality contributions.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Big +1 for Kristen, she has been an active member of the community for a long time and would be a great asset to the group.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Alternative approach in MR!6824. There are only two services that fit the pattern (and unlikely to be many more in contrib), performance doesn't seem super important here, so maybe we can just use a service collector? This requires the least code changes and as OpenerResolver is internal we can do this without providing BC - unsure why someone would want to replace this anyway.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Applied the patch locally, I can't find any remaining deprecated code in the diff component.

πŸ‡¬πŸ‡§United Kingdom longwave UK

This simpler approach seems to work. Unsure if this needs BC - are middleware services part of the API?

πŸ‡¬πŸ‡§United Kingdom longwave UK

longwave β†’ changed the visibility of the branch 3420215-remove-containerawaretrait-from to hidden.

πŸ‡¬πŸ‡§United Kingdom longwave UK

As dead code IMHO this can go back to 10.3.x.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Maybe we just postpone this until we have figured out what to do with ClassResolver?

πŸ‡¬πŸ‡§United Kingdom longwave UK

This is only in tests, no CR needed.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Added https://www.drupal.org/node/3424376 β†’ though I think it is super unlikely anyone will be affected by this, it was only added for KernelTestBase and is unused now for over six years.

πŸ‡¬πŸ‡§United Kingdom longwave UK

The constructor will ensure the new properties are always set, so $this->container is no longer needed at all?

πŸ‡¬πŸ‡§United Kingdom longwave UK

Yup we should also do a quick dependency evaluation and add it to https://www.drupal.org/about/core/policies/core-dependency-policies-and-... β†’ (seems the existing packages are not there, but that shouldn't stop us adding these ones)

@justafish I assume you/Lullabot are OK to continue maintaining these packages as needed and also handle any security reports?

πŸ‡¬πŸ‡§United Kingdom longwave UK

πŸ“Œ Add declare(strict_types=1) to all Functional tests Fixed landed and all child issues are completed, I think we are done here - thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Committed and pushed to 11.x and 10.3.x, thanks!

PHPStan crashed locally when running against this change via commit-code-check.sh with lots of "error code 127", unsure if there were too many changes for it to handle in one go?

πŸ‡¬πŸ‡§United Kingdom longwave UK

Yeah no CR is needed, no API is changing here really, we are just copying the method and property directly into the class.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Not sure I'm taking the right approach here. Maybe we just inject a session closure directly into the session middleware?

πŸ‡¬πŸ‡§United Kingdom longwave UK

Thanks, I did not see that issue earlier. I think that one can be closed as duplicate because:

  • That one defines a new (but unused) queue.memory service, which I don't see the point in
  • That one also adds a return type, but we can't do that without breaking backward compatibility

Otherwise the implementations are the same except for comments. Crediting fgm here for the work done over there.

πŸ‡¬πŸ‡§United Kingdom longwave UK

As the request stack and current user are now always available, we can always inject them into the created LoggerChannel.

πŸ‡¬πŸ‡§United Kingdom longwave UK

session_handler needs to be explicitly autowired.

πŸ‡¬πŸ‡§United Kingdom longwave UK

ChainedFastBackendFactory follows the same pattern. Have not figured out how to work around the issue in #5, so the simplest solution is just to continue injecting the container into both factory services.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Looks like this is no longer useful.

πŸ‡¬πŸ‡§United Kingdom longwave UK

DrupalKernel::initializeContainer() does this:

    // If we haven't booted yet but there is a container, then we're asked to
    // boot the container injected via setContainer().
    // @see \Drupal\KernelTests\KernelTestBase::setUp()
    if (isset($this->container) && !$this->booted) {
      $container = $this->container;
    }

but KernelTestBase::setUp() hasn't called setContainer() since 2017: #2880911: Remove unused KernelTestBase::getCompiledContainerBuilder() β†’

Maybe this is just dead code?

πŸ‡¬πŸ‡§United Kingdom longwave UK

Following πŸ“Œ Fix install container definition of StreamWrapperManager Needs review I think we can simplify this.

Unsure of the value of a change record here. The likelihood that someone is overriding the entire logger factory seems low to me. But for completeness it won't take long to write one.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Agreed with @mstrelan that although we tried to fix all strict errors in πŸ“Œ Fix strict type errors in WebAssert calls Needs review one more has snuck in since; as it's only one instance the most pragmatic way to continue is just to apply that fix directly as part of this issue.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Backported to 10.2.x as a test-only fix to keep things in sync.

Committed and pushed c4e7dd71b6 to 11.x and d5d95b0411 to 10.3.x and f9d3970b7b to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

11.x is open for development so this can be worked on now.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Many tests are failing.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Tagging for the framework managers to answer #93 but other than that I think we are done here, thank you for all your work on getting this in!

πŸ‡¬πŸ‡§United Kingdom longwave UK

We need to add a deprecation notice to Html::escapeCdataElement() and write a test that covers it. We also need a change record that documents the deprecation.

See https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... β†’ for how to deprecate the method, and you can find existing deprecation tests in core that you can copy and alter for this new deprecation.

πŸ‡¬πŸ‡§United Kingdom longwave UK

#56 makes sense to me, we have those helpers so why not use them.

πŸ‡¬πŸ‡§United Kingdom longwave UK

I have been watching this issue for a long time and never commented because I am very on the fence about it. On the one hand I see that we would move faster and fix more bugs if we committed some fixes without tests, but on the other I am worried about introducing regressions later on. But after sitting on it for a while I think the proposal here is a good compromise to both those arguments.

I also agree with #100/102 that update tests are one of the most painful things to write, and concur that if the update hook is trivial then we should not require explicit testing; we do have test coverage that ensures that all updates are at least successful, and we have test coverage for e.g. the config/state APIs, so there seems little value in ensuring that an update hook can correctly make a minor change via those APIs. I further agree with #103 that CSS fixes and some accessibility fixes really can't be tested.

Overall, the suggestion in the IS that the majority of the questions must be answered yes is a good failsafe. However:

  1. Is the fix is easy to verify by manual testing?
  2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc.
  3. Is the fix achieved without adding new, untested, code paths?
  4. Is an explicit 'regression' test needed?
  5. Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?
  6. Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?
  7. If this fix is committed with test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?

Question 4 stands out to me here. It's either the wrong way round - Is no explicit regression test needed? Yes means we can proceed without a test - or perhaps even it can be removed as a duplicate of question 7?

The first part of question 6 also suffers from a similar problem, and I think "with" should be "without" in question 7.

Suggested rewording where, to me, primarily "yes" answers point towards not requiring test coverage:

  1. Is the fix is easy to verify by manual testing?
  2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc.
  3. Is the fix achieved without adding new, untested, code paths?
  4. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?
  5. Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?
  6. If the issue exposes a general lack of test coverage for the specific subsystem, is it better to add generic test coverage for that subsystem in a separate issue?

I moved the final regression question higher as to me that is more important than the latter two questions.

I also agree with #104 in that we could use "Needs no tests" when a decision has been made after answering the questions, or "Needs tests" if we decide the opposite, and also in conjunction with "Needs followup" if we decide to push test coverage to a separate issue (which should be opened and the tag removed before the bug fix is committed).

πŸ‡¬πŸ‡§United Kingdom longwave UK
if (!$this->isNewRevision() && isset($this->original) && (!isset($record->revision_log) || $record->revision_log === '')) {

I also think this logic could be simplified:

if (!$this->isNewRevision() && isset($this->original) && isset($record->revision_log) && $record->revision_log === '') {

If $record->revision_log is not set then we don't need to set it at all for the purposes of the SQL statement.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Just run into this in a roundabout way. I have a site which has altered the revision_log field and $record->revision_log is not a valid column in the table any more. Agree that this is making assumptions about the data structure that is in use here, and it should be done in a more generic way instead.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Via @alexpott in Slack

Been thinking about the deprecation - wanna move it to somewhere better.

πŸ‡¬πŸ‡§United Kingdom longwave UK

The code changes are out of scope. We should only be touching that docblock here.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Looking at it again, decided to revert; let's add a default value for that new argument so existing callers don't break.

πŸ‡¬πŸ‡§United Kingdom longwave UK

As far as I can tell this is dead code since πŸ› Upgrade filter system to HTML5 Fixed , previously we ended up with scripts/styles wrapped in CDATA but this is no longer required in HTML5 and tests were updated over there to remove CDATA, so I think no new tests are needed here either (except maybe a deprecation test, I guess).

πŸ‡¬πŸ‡§United Kingdom longwave UK

Whoops, I thought I searched contrib and found no uses, maybe an error on my part. Happy to revert and rework it if that solves the issue.

πŸ‡¬πŸ‡§United Kingdom longwave UK

I suppose a third option is to add a checkbox to FromEmailAdjuster which also sets the sender?

πŸ‡¬πŸ‡§United Kingdom longwave UK

+1 to the approach and the patch looks good but I haven't had time yet to try and reproduce again with XDebug enabled.

@solideogloria @Driskell if you can confirm that this latest set of patches fix your issues here that would be helpful to getting this committed.

πŸ‡¬πŸ‡§United Kingdom longwave UK

system.rss.yml is mostly useless and should be removed in πŸ› Views RSS view mode settings are completely broken Needs work

πŸ‡¬πŸ‡§United Kingdom longwave UK

@epieddy https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... β†’

The relevant quote is

We reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to:
...
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

There is not quite a 1:1 relationship here, because both \Drupal\Core\Database\Query\Select and \Drupal\Core\Database\Query\SelectExtender implement \Drupal\Core\Database\Query\SelectInterface. But I think it is OK to add the method to both those base classes, because nobody else should be implementing SelectInterface directly, they should always be extending from one of those two implementations.

(@daffie please correct me if I have missed something here)

πŸ‡¬πŸ‡§United Kingdom longwave UK

Committed c2f5752 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Backported to 10.2.x as it cherry-picked cleanly and if we need to add headers in the future then it will be useful to have there.

Committed and pushed 4356343b4e to 11.x and 317f5ecdb1 to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

StandardPerformanceTest is consistently failing with this change.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Would be nice if we could prevent these creeping back in, I think PHPStan can do this at higher levels but we aren't quite ready for that yet.

-  $kernel->rebuildContainer(FALSE);
+  $kernel->rebuildContainer();

Wondered about the history of this one and this was wrong when it was first introduced, rebuildContainer() has never taken a boolean argument.

πŸ‡¬πŸ‡§United Kingdom longwave UK

We could also consider this "works as designed", if you're running a -dev version of core you are on your own anyway and there is no guarantee of an update path (we might have reverted things from the specific version you are using, for example).

πŸ‡¬πŸ‡§United Kingdom longwave UK

Added some suggestions to the MR.

πŸ‡¬πŸ‡§United Kingdom longwave UK

The one issue I see is that when we switch to main or open a new branch for core development, what happens when composer require drush/drush drupal/config_inspector starts failing because there is no compatible package yet? Given this runs on any change to a .module file then this is likely to make MRs on a new branch fail even if they are nothing to do with config schema.

But, perhaps we just deal with that when it happens by altering the job?

πŸ‡¬πŸ‡§United Kingdom longwave UK

There is not enough information to diagnose the problem, and automated tests would catch this if it happened in a basic install of Drupal.

If you have a full stack trace, that might be helpful. And if you can reproduce this starting from a fresh install of Drupal, please write down the steps required to create the error.

πŸ‡¬πŸ‡§United Kingdom longwave UK

There is also an interesting choice to make here regarding public vs private services. Drupal services are still public by default; Symfony switched to private by default some time ago. And when you use resource in Symfony:

Wait, does this mean that every class in src/ is registered as a service? Even model classes? Actually, no. As long as you keep your imported services as private, all classes in src/ that are not explicitly used as services are automatically removed from the final container. In reality, the import means that all classes are "available to be used as services" without needing to be manually configured.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Thanks for fixing the BC layer in the constructor.

I added a question about the library BC layer - not sure strtr() does what you expect here, as it replaces bytes in a string?

Going to ask the framework managers for input on:

  • Whether to have one combined TwigExtension class or keep the components TwigExtension as a separate service
  • Whether to add class aliases or if we are OK just to name services with their FQCN
πŸ‡¬πŸ‡§United Kingdom longwave UK

I have a requirement to set a different sender for certain emails, so I needed to be able to change this via policy. I added a simple plugin to custom code to do so, I've ported it here as part of Symfony Mailer itself if this is useful.

πŸ‡¬πŸ‡§United Kingdom longwave UK

longwave β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡§United Kingdom longwave UK

The more I think about it the more I wonder what is a plugin manager, if not much more than a service container/locator that only contains a subset of services that conform to a specific interface?

πŸ‡¬πŸ‡§United Kingdom longwave UK

In services this can be done with aliases; you create the new service and alias the old service name to it. Aliases can be deprecated too. Do we need this concept in plugins too?

πŸ‡¬πŸ‡§United Kingdom longwave UK

This sounds like a really nice feature. If we're going to bring in symfony/config we should just do ✨ Use native Symfony YamlLoader + Config Needs work and then extend Symfony's YamlFileLoader to add our own features, if we can - but there was quite a lot of resistance to adding that dependency over there, for one because it starts to imply that we use other features of Symfony's configuration system, but Drupal does things entirely differently. Is there any way we can do this or πŸ“Œ Support resource key in services.yml Active without adding the dependency?

πŸ‡¬πŸ‡§United Kingdom longwave UK

@joachim in a way that needs to be understood by code and handled automatically, or only in a message to be read by humans?

πŸ‡¬πŸ‡§United Kingdom longwave UK

Also how will this work with plugin types where we intend to use variadics already to handle any number of additional properties? (such as entity types)

πŸ‡¬πŸ‡§United Kingdom longwave UK

We could also detect future missing properties at discovery time by reflecting the plugin type class and raising a deprecation if an expected constructor argument (such as $deprecation or whatever else we add) is missing.

πŸ‡¬πŸ‡§United Kingdom longwave UK

In two minds about this. On one hand this future proofs us for adding more base properties. On the other hand the only reason we are doing this is that since the invention of plugins when we had two properties, we are only now looking to add a third, so is this adding DX complexity for something we might never use again?

πŸ‡¬πŸ‡§United Kingdom longwave UK

The comments from #26, #46, #47 are still valid. If @znerol still has the script that was mentioned that would help here, otherwise we need to port the script or write something new.

Leaving at NR in the meantime to try and get more eyes on this.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Tried an alternative approach for CommentLinkBuilderTest in MR!6661.

MailHandlerTest is a bit more messy but I think we could clean it up to make the test more readable at the same time.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Updated the IS. I don't think we need any additional testing but profiling the difference in instantiation time when a large number of subscribers are in use would be helpful.

πŸ‡¬πŸ‡§United Kingdom longwave UK

There is not really enough information to help you here. You say the bug is reproducible when aggregation is disabled which is interesting and suggests that perhaps there is a problem in your JS or CSS that is being accidentally solved when aggregation is enabled again.

You need to try and identify what is different about the sections that are not working. Also are there any errors in the Drupal logs or the JavaScript console?

πŸ‡¬πŸ‡§United Kingdom longwave UK

Yes, whoops, done that as well - thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Was on the fence about backporting this as it adds a tiny bit of API but this seems unlikely to be unused outside of the ConfigImporter and if backporting allows us to unblock contrib then let's do that.

Committed and pushed eafa856443 to 11.x and 476e65a1e1 to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK
is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0

What is the reason for keeping this until Drupal 12? We are OK to deprecate in 10.3.0 for removal in 11.0.0, except where we think things might be widely used or difficult to remove, but this one looks straightforward.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Looks straightforward.

Committed 5294815 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Also the comment in ajax.js still says:

@return {object}
  Returns the jQuery.Deferred object underlying the Ajax request. If
  pre-serialization fails, the Deferred will be returned in the rejected
  state.

This is no longer true, and I also wonder about backward compatibility here.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Looks like this might break contrib. Toolbar Menu has nearly 7000 installs and uses Drupal.toolbar.setSubtrees which is refactored here.

https://git.drupalcode.org/project/toolbar_menu/-/blob/8.x-2.x/js/toolba...

If we can't add backward compatibility then we need a change record and ideally should create an issue in their queue to notify them that we are changing this API.

πŸ‡¬πŸ‡§United Kingdom longwave UK

It doesn't make a huge amount of difference, but we should show consistency for others to follow.

Committed 63cfce4 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Committed e853205 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Wondered if it was worth making this extensible or swappable but I searched the issue queue and found no issues discussing either of these, so I think this is fine just to move to a static utility class, especially given each method only has exactly one usage in core.

Committed c936bda and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Issue credit did not save correctly.

πŸ‡¬πŸ‡§United Kingdom longwave UK

Almost didn't commit this because of the change to views.view.test_statistics_integration.yml but on further investigation we have already committed cspell:ignore to test config already. I do think we should try to avoid this if at all possible in "real" module config though.

Committed cb328b8 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom longwave UK

Backported to 10.2.x as a test-only fix.

Committed and pushed 9179fd28e3 to 11.x and 26e733a1e2 to 10.2.x. Thanks!

Production build https://api.contrib.social 0.61.6-2-g546bc20