🇬🇧United Kingdom @Eli-T

We should fix the phpstan warnings in this MR.

Good spot @mrinalini9 ! Looks like this was fixed in 🐛 cache.jsonapi_resource_types can cause problems if a resource type is installed afterwards Active by @alexpott

Patch no longer applies since upgrading from 8.x-1.1 to 8.x-1.2.

Here's a patch rerolled against 2.1.4.

Patch does also apply cleanly to 2.1.3.

Patch no longer applies after updating from flysystem_s3 2.1.2 to 2.1.4.

Is 10.4.7 definitely the issue here?
Have you tried this module on other versions of Drupal and it worked for you? If so, which ones?

Eslint is now happy in Github CI.

A second pair of eyes on the JS changes would be appreciated!

The automated fix patch generated by eslint has fixed some but not all of the issues.

The remaining issues are:

/builds/project/form_state_empty/web/modules/custom/form_state_empty/js/state-empty.js
  10:29  error  Expected '===' and instead saw '=='    eqeqeq
  14:13  error  Prefer HTMLInputElement#value to .val  no-jquery/no-val
✖ 2 problems (2 errors, 0 warnings)

eli-t → created an issue.

Thanks everyone!

eli-t → made their first commit to this issue’s fork.

2.0.x branch is already D11 compatible.

Whilst there is an MR, all it does is makes a return type explicit, which is not required for Drupal 11.

@maztab the state of the issue is the state of the issue.

It needs work.

The current problem is that stated in #12.

Contributions welcome!

Well done everyone! We got there in the end.

New release 3.0.0 coming imminently.

Adding credits

This issue is due to flysystem_s3 swapping out the deprecated \Drupal\flysystem\Plugin\ImageStyleGenerationTrait::generateImageStyle() for its nominated replacement generateImageUrl().

generateImageUrl() relies on the presence of the flysystem.image_style_redirect.serve route, which doesn't appear to always be dynamically generated:

return \Drupal::urlGenerator()->generate('flysystem.image_style_redirect.serve', $args, UrlGeneratorInterface::ABSOLUTE_URL);

flysystem_s3 will have to revert to the old function, and inform phpstan to ignore this deprecation.

This will stop flysystem_s3 from being compatible with flysystem 3.0.x until it is resolved.

The composer.json and info.yml Drupal core constraints got out of sync - let's fix that here as well.

Ignore that - that was done a month ago!

The composer.json and info.yml Drupal core constraints got out of sync - let's fix that here as well.

Setting up with Drupal 10, flysystem 2.2.0-beta2 and flysystem_s3 2.1.3 and exactly the same settings.php configuration for flysystem works without the issue in #3430648-31: Automated Drupal 11 compatibility fixes for flysystem_s3 → so this doesn't look like config, and either an outstanding D11 issue in flysystem or flysystem_s3.

Testing with flysystem 2.3.x and this MR, I can successfully upload image field images to S3.

However when trying to view such images I get error below.

Not sure if this is just a config issue on my test setup or a genuine D11 issue.

The website encountered an unexpected error. Try again later.

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "flysystem.image_style_redirect.serve" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 208 of core/lib/Drupal/Core/Routing/RouteProvider.php).
Drupal\Core\Routing\UrlGenerator->getRoute() (Line: 269)
Drupal\Core\Routing\UrlGenerator->generateFromRoute() (Line: 105)
Drupal\Core\Render\MetadataBubblingUrlGenerator->generateFromRoute() (Line: 96)
Drupal\Core\Render\MetadataBubblingUrlGenerator->generate() (Line: 94)
Drupal\flysystem_s3\Flysystem\S3->generateImageUrl() (Line: 227)
Drupal\flysystem_s3\Flysystem\S3->getExternalUrl() (Line: 75)
Drupal\flysystem\FlysystemBridge->getExternalUrl() (Line: 103)
Drupal\Core\File\FileUrlGenerator->doGenerateString() (Line: 66)
Drupal\Core\File\FileUrlGenerator->generateAbsoluteString() (Line: 255)
Drupal\image\Entity\ImageStyle->buildUrl() (Line: 302)
.
.
.

Looks like the deprecations about which phpunit was complaining were in the custom phpunit.xml.dist file supplied with this project.

The merge pipeline now passes with that file removed.

PHPunit says we have 10 tests and 26 assertions all passing with this removed.
These are the same totals as before the phpunit.xml.dist was removed, so it looks like this file was unnecessary.

I am not aware of any more issues with this merge request.

wouldn't setupBeforeClass() be a better replacement for __construct() than setup()?

No, it appears not. setupBeforeClass() is static therefore not appropriate for setting class properties.

@davidlfg wouldn't setupBeforeClass() be a better replacement for __construct() than setup()?

https://docs.phpunit.de/en/9.6/fixtures.html?highlight=setupbeforeclass

eli-t → made their first commit to this issue’s fork.

Here is an MR that does not alter current behaviour but allows you to set a boolean config value enqueue_warmers_on_cron_run, which when set to FALSE, will stop warmer from enqueuing all the warmers every time cron runs.

There are CI issues for this MR but they are not related.

eli-t → made their first commit to this issue’s fork.

OK, but I had already made that change in the MR before either of you commented about it. So the error message must be due to something else.

Apologies - I didn't mean to be confusing.

The case change was unrelated to the PHPUnit failure - it was to do with failed PHPCS run in the validate stage of the pipeline after commit b19950b8 : https://git.drupalcode.org/issue/flysystem_s3-3430648/-/jobs/5477376

The validate stage is now passing again following commit a7d997cb .

Thanks for all the ongoing work on this.

Just want to remind folks we need to use FALSE and NULL rather than false and null for coding standards!

I've tried to push this along as far as I can today.

The only issue I'm currently aware of stopping this being merged and having a D11 release is that the PHPUnit job in CI is currently failing.

For example https://git.drupalcode.org/project/flysystem_s3/-/jobs/5464727

All the tests themselves pass, so I presume the job failing is due to the deprecations. I can't see the specifics of those in CI, but running locally I can see

❰elliot.ward❙~/code/d11test/d11/web/modules/contrib/flysystem_s3(git:3430648-d11_ready)❱✔≻ ddev exec phpunit -c web/core/phpunit.xml.dist web/modules/contrib/flysystem_s3
HTML output directory sites/simpletest/browser_output is not a writable directory.

PHPUnit 10.5.36 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.21
Configuration: /var/www/html/web/core/phpunit.xml.dist

DD.D......                                                        10 / 10 (100%)

Time: 00:02.496, Memory: 20.00 MB

3 tests triggered 5 deprecations:

1) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" might add "array" as a native return type declaration in the future. Do the same in implementation "Drupal\flysystem\ImageStyleCopier" now to avoid errors or add an explicit @return annotation to suppress this message.

Triggered by:

* Drupal\Tests\flysystem_s3\Functional\ModuleInstallUninstallWebTest::testInstallationAndUninstallation
  /var/www/html/web/modules/contrib/flysystem/tests/src/Functional/ModuleInstallUninstallWebTest.php:27

2) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "Aws\CacheInterface::get()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Drupal\flysystem_s3\AwsCacheAdapter" now to avoid errors or add an explicit @return annotation to suppress this message.

Triggered by:

* Drupal\Tests\flysystem_s3\Unit\AwsCacheAdapterTest::testBasicGetSetDelete
  /var/www/html/web/modules/contrib/flysystem_s3/tests/src/Unit/AwsCacheAdapterTest.php:20

3) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "League\Flysystem\AwsS3v3\AwsS3Adapter::has()" might add "bool" as a native return type declaration in the future. Do the same in child class "Drupal\flysystem_s3\Flysystem\Adapter\S3Adapter" now to avoid errors or add an explicit @return annotation to suppress this message.

Triggered by:

* Drupal\Tests\flysystem_s3\Unit\Flysystem\S3Test::testGetExternalUrl
  /var/www/html/web/modules/contrib/flysystem_s3/tests/src/Unit/Flysystem/S3Test.php:61

4) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "League\Flysystem\AwsS3v3\AwsS3Adapter::getMetadata()" might add "false|array" as a native return type declaration in the future. Do the same in child class "Drupal\flysystem_s3\Flysystem\Adapter\S3Adapter" now to avoid errors or add an explicit @return annotation to suppress this message.

Triggered by:

* Drupal\Tests\flysystem_s3\Unit\Flysystem\S3Test::testGetExternalUrl
  /var/www/html/web/modules/contrib/flysystem_s3/tests/src/Unit/Flysystem/S3Test.php:61

5) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "League\Flysystem\AwsS3v3\AwsS3Adapter::upload()" might add "array|bool" as a native return type declaration in the future. Do the same in child class "Drupal\flysystem_s3\Flysystem\Adapter\S3Adapter" now to avoid errors or add an explicit @return annotation to suppress this message.

Triggered by:

* Drupal\Tests\flysystem_s3\Unit\Flysystem\S3Test::testGetExternalUrl
  /var/www/html/web/modules/contrib/flysystem_s3/tests/src/Unit/Flysystem/S3Test.php:61

OK, but there were issues!
Tests: 10, Assertions: 27, Deprecations: 5.

These deprecations do not seem to be raised on D10 / flysystem_s3 2.1.x / phpunit 9.

I probably won't get time to look at this again in the nearish future, so if anyone is requiring a D11 release, the best thing you can do to move this issue forward is to look at why PHPunit is failing in CI.

We should do this for the 2.1.x branch as well

eli-t → created an issue.

On second thoughts, using 2.1.x for D10 and 2.2.x for D11 prevents us from ever having another D10 minor release, which is something we may possibly want to do. So let's use 3.0.x for D11.

Patch in #6 unfortunately does not apply against 8.x-1.5.

Hi Finn! I agree that getting more councils access to a potentially free government provided email service would be a great thing. I started (with @ekes) to look at this at dev days.

It doesn't really make sense to make govuk compatible with symfony mailer; the two modules fulfil the same function.

I started to look at localgov_workflows_notifications to see if I could make it fall back to use the default mailer when symfony_mailer is not installed at https://github.com/localgovdrupal/localgov_workflows/issues/131 but it's more work than I can take on for a project I'm not involved with at the moment.

I'm going to close this issue due to the above.

Thanks,

Eli

Hi! I'm closing this as I don't think we need it since 📌 Fix PHPstan issues Active was merged. But please feel free to open it again if you think it's still needed.

eli-t → created an issue.

eli-t → created an issue.

eli-t → made their first commit to this issue’s fork.

There are two issues with this proposal at the moment.

1) it doesn't take account of the use_relative_urls configuration setting for the module
2) it breaks when viewing the content through JSON:API; that is the breadcrumbs refer to the JSON:API path

1) is not surprising as the initial work on this issue was done before that option was merged.

However, 2) is more of a fundamental problem with this approach - this is a breaking change, and it is difficult to see how this would ever be the wanted behaviour when viewing content through JSON:API.

For example - here is the jsonapi output on the main branch for an example page using absolute links

breadcrumbs	
  0	
    uri	"https://recommended-project.ddev.site/"
    title	"Home"
    options	[]

With this change it looks like this

breadcrumbs	
  0	
    uri	"https://recommended-project.ddev.site/"
    title	"Home"
    options	[]
  1	
    uri	"https://recommended-project.ddev.site/jsonapi"
    title	"Jsonapi"
    options	[]

As far as I can establish, it has never been the purpose of this module to display the computed breadcrumbs. Therefore we should prevent such fields from being displayed.

We can stop the base field from appearing on this screen by setting

setDisplayConfigurable('view', FALSE)

and removing the call to setDisplayOptions() in computed_breadcrumbs_entity_base_field_info().

However, this does not stop configurable fields of this type from appearing on the Manage Display form.

I cannot think of a valid use for having multiple instances of this field type on an entity so propose setting no_ui = true on the on the notation for ComputedBreadcrumbsLinkItem. This will prevent users from creating additional computed fields.

However this change of behaviour is enough to warrant a new major release, so creating a new 2.0.x branch and moving this change to that branch.

Update to latest branch

I think this change is a really good idea.
The original MR 13 was done on the 1.1.x branch rather than an issue specific branch, so I've copied that modified 1.1.x branch to 3426428_v2 branch, merged in the 1.2.x branch and fixed the merge conflicts.

It looks like we do now have test failures to address before we can proceed.

I'll take a look at this today and issue a new release if all is well.

baikho → credited eli-t → .

baikho → credited eli-t → .

Updated Guzzle Client 6 to 7.

Updated text now Australian platform is no longer available.

This is a duplicate of 📌 Drupal 11 Compatibility Active which has some actual work done on it.

@quietone I've just tested this on Drupal 11.0.9 and can confirm the problem exists there too.

Actually setting the status to Active rather than Needs work as explicitly requested by @quietone in #54

Reopening.

Here is a video showing the issue starting from a Standard install of 10.3.9 on SimplyTest.me

https://www.youtube.com/watch?v=CZBmAEukp5g

If we address the PHPCS issue and folks falling foul of this confirm it works, let's get it merged and in a release - it's been hanging around long enough!

Thanks @davidwhthomas!

MR!21 is showing PHPCS failures so putting back to needs work.

https://git.drupalcode.org/issue/views_ef_fieldset-2497845/-/jobs/3481155

We came across this converting a site to multilingual - we had a limit in services.yml for /jsonapi/* routes but not /cy/jsonapi/*.

This patch fixes that issue.

(we're going to add a limit for the other language anway).

Thanks everyone!

eli-t → made their first commit to this issue’s fork.

This looks good, moving to RTBC and adding credit

@viniciusrp can you please update the merge request instead of posting patches? That will run the CI.

@viniciusrp can you please update the merge request instead of posting patches? That will run the CI.

You're welcome @evilargest! Thanks for checking and letting me know 💙

Further to #10, in Drupal Slack #support channel, @godotislate pointed me towards \Drupal\views\Plugin\views\filter\LanguageFilter::access(), which is what stops the langcode exposed filter from showing.

Apologies, current MR conflicts with 8.x-1.x HEAD

@evilargest can you confirm if this is still an issue with the latest 8.x-1.x please?

Apologies - merge request now conflicts with HEAD of 8.x-1.x.

Marking as needs work as question in #6 unanswered, and patches should now be converted to merge requests.

Apologies, patch in #10 no longer applies.

Whilst I'm not opposed to the code quality improvements in this patch, they should be addressed in their own separate issues rather than here where reasonably practical.

Can you confirm if this is still an issue on HEAD of 8.x-1.x please?

These docblocks have now been removed elsewhere.

Apologies, MR now conflicts.

Thanks for all your work everyone 💙