🇬🇧United Kingdom @Eli-T

Update code examples to use short array syntax as per coding standards:  https://www.drupal.org/docs/develop/standards/php/php-coding-standards#a... →

Can I also suggest updating the default branch in gitlab to 2.x?

Thanks for this module, I definitely think this should be part of core's status report.

Eli-T → changed the visibility of the branch 3455891-version_mismatch to hidden.

Eli-T → created an issue.

Eli-T → created an issue.

Thanks for testing @rodrigoaguilera 💙

New release supporting Drupal 11 available at https://www.drupal.org/project/uploaded_file_filename_randomizer/release... →

Just to confirm following discussion with @lhridley in Slack that there are no known compatibility issues between flysystem 2.2.0-alpha3 with 10.3 at this time.

I have tested flysystem alongside flysystem_s3 on Drupal 10.3.0 beta 1 and have not found any issues, but the title of this issue suggests further work is required on flysystem to make it 10.3 compatible.

Are there any known compatibility issues between flysystem with 10.3 at this time?

Closing this issue now as we have refactored our custom module to decorate MenuItemsResource rather than extend it. This works with no changes to this module. Note you need to implement getRouteResourceTypes() on the decorator class, and in response call it on the decorated class if you do this.

On a tangential note, whilst diffing 1.2.4 and 1.2.5 to try to figure out what had changed, it felt more like this could have been a minor release than a patch release!

Looks like #3270141-5: Allow other modules to extend MenuItemsResource → isn't quite enough to successfully subclass MenuItemsResource in 1.2.5.

In our case, this broke because in 1.2.5, MenuItemsResource now implements ContainerInjectionInterface.

When \Drupal\jsonapi_resources\Unstable\Controller\JsonapiResourceController::processRequest() is handling a request for the subclassed resource, it calls $this->getJsonapiResource(), which ultimately uses \Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition() to get the class.

This part of that function now has different behaviour

  if (is_subclass_of($definition, 'Drupal\Core\DependencyInjection\ContainerInjectionInterface')) {
    $instance = $definition::create($this->container);
  }
  else {
    $instance = new $definition();
  }

because in 1.2.5 we pass the if condition, unlike 1.2.4.

And because \Drupal\jsonapi_menu_items\Resource\MenuItemsResource::create() returns new self(), even when $definition is set to our subclass, the parent class is returned.

Therefore jsonapi_resources acts as if the parent class was requested rather than the subclass.

Here is an addition to the patch in #3270141-5: Allow other modules to extend MenuItemsResource → that changes self to static. I presume this will annoy PHPStan. However, this makes inheritance possible here and works with our use case. I'd be grateful to hear of any better approaches though!

@bushra-shaikh have you actually tested the functionality of this patch, or just checked the upgrade status report?

Adding credit

Closing in favour of 🐛 Options using the same "search attribute"'s value are not all available Needs review

Note the issue raised in #10 is now resolved in 2.0.x.

@arousseau does this fix work for you? Technically it's a BC break in case anyone was relying on the shape of this response, but as the module is alpha and has low usage I doubt this will cause problems for people.

Eli-T → created an issue.

Good spot - just tested on 2.0.x and can recreate.

Have created 📌 Add tests for the autocomplete widget Active for the autocomplete tests, so as to not hold back this issue.

Eli-T → created an issue.

All tests removed from Drupal CI now.

Thanks @alexpott!

Merge conflicts to 2.0.x resolved.

Added stack trace of error generated on simplytest.me to issue summary.

Thanks for contributing! I should have time to take a look at this next week.

Update of the patch in #18 rerolled against 2.0.0-alpha2

Awesome! Good luck with the Backdrop version 💙

Closing this issue as there's nothing to do here.

This was merged in https://git.drupalcode.org/project/uploaded_file_filename_randomizer/-/c...

I'm not sure why the issue doesn't show that.

Eli-T → created an issue.

Eli-T → created an issue.

Eli-T → created an issue.

Just fixing what looks like copy-paste hangovers in the summary - Standard ➡️ Umami in text and standard ➡️ demo_umami in config links.

@mkolar do you need to use the 3.x.x branch? Flysystem 2.2.x on Drupal 10.2.x with the patch in 🐛 CssCollectionOptimizerLazy and JsCollectionOptimizerLazy use removed inherited member state Needs review seemed ok for us in testing, but we haven't rolled out to production yet.

One thing to consider here is if the code the MR removes is still required in < 10.2, and whether we should check versions or actually enforce > 10.2 in the next releases composer.json. I'd be interested in what the module maintainers opinions are there.

Eli-T → created an issue.

Just to add we saw this after upgrading to Drupal 10.2.2.

Moving from DER 3.1.0 to dev branch fixes the issue.

I am sceptical this happened because of corrupt config for the field, because if I create a duplicate field with the same options, the exported configuration is identical (except for field name etc).

If it's of interest, the configuration is publicly visible at

https://github.com/ministryofjustice/prisoner-content-hub-backend/blob/m...
https://github.com/ministryofjustice/prisoner-content-hub-backend/blob/m...
https://github.com/ministryofjustice/prisoner-content-hub-backend/blob/m...

Eli-T → created an issue.

Yes

@luigisa are you definitely on the latest 2.2.x? This error does not happen for me with b80a666.

I created 🐛 CssCollectionOptimizerLazy and JsCollectionOptimizerLazy use removed inherited member state Needs review to delete the state references from JsCollectionOptimizerLazy and CssCollectionOptimizerLazy as this issue is closed.

Eli-T → created an issue.

Now I've had a coffee I realise you don't need to add CKEditor5 as an additional project with Drupal 9.5.11. However, the error message here is still confusing so I'll leave the issue open and let the maintainers decide whether to close it.

Eli-T → created an issue.

For some reason* the patch generated on gitlab for the current state of this merge request won't apply against current 2.1.x, but a manually generated diff between the merge request branch and 2.1.x will. This is v odd especially as the MR will merge without conflicts.

So here's a patch.

*probably because I somehow did something wrong merging in the changes commit in 📌 Fix PHPCS and PHPStan issues reported by Gitlab CI Fixed 🤷‍♀️

Apologies, my PHPStorm settings have mangled the formatting in that push. Will rectify forthwith...

Needs reroll to apply against 2.1.x following [#3409513]

Tested with no regressions found. Merged to 2.1.x.

@2dareis2do you can get the patch for a merge request by appending .patch to the URL of the MR.

EG for https://git.drupalcode.org/project/flysystem_s3/-/merge_requests/15 you can get the patch at https://git.drupalcode.org/project/flysystem_s3/-/merge_requests/15.patch

@2dareis2d0 because that's how core changes are made according to policy: https://www.drupal.org/about/core/policies/core-change-policies/backport... → , and https://www.drupal.org/about/core/policies/core-change-policies/backport... →

If you disagree specifically with this policy, this specific issue is probably not a helpful place to have that discussion.

Moving back to needs review as I'd like someone using the module to check it for regression. There's no evidence that the module has actually been tested yet.

It's not the dependencies for PHPStan that are the issue here, it's the dependencies for flysystem_s3 that are missing.

@thakurnishant_06 looks like you haven't installed the composer dependencies?

@thakurnishant_06 If you are definitely running against the correct code can you please share your full phpstan.neon and your output of running phpstan?

Thanks @thakurnishant_06

Are you sure you are running phpstan against the fork used by the merge request?

Running locally with level 0 I get

~/code/d10stans3 via 🐘 v8.1.13 on ☁️  (eu-west-1) took 4s
❯ vendor/bin/phpstan
Note: Using configuration file /Users/eli/code/d10stans3/phpstan.neon.
 11/11 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%



 [OK] No errors

hestenet → credited Eli-T → .

With respect to #3409514-3: Fix PHPCS and PHPStan issues reported by Gitlab CI → and having read Drupal's BC policy especially concerning constructors being considered internal, I think we can resolve all the PHPCS and PHPStan issues without forcing a new release.

MR 15 fixes the PHPCS issues.

I'm thinking about splitting this issue as fixing all the PHPStan issues will necessitate potentially breaking backwards compatibility.

Eli-T → created an issue.

Tidied up, tested, and merged!

Thanks everyone - looks like this should work with the core functionality in 10.2.x automatically, as soon as we have a version of flysystem that supports it.

Thanks for the rebase @mkhamash!

It looks like this introduces new phpcs issues that we would need to resolve.

There are phpstan warnings too but it doesn't look like they were introduced by this patch, I guess the default Gitlab CI is doing more now than when it was first introduced.

Can I ask why you uploaded a patch as well as updating the MR? I'm not sure that's necessary.

Well I can't change what's in 8.x-1.6 but I can give you a shiny new 8.x-1.7 with this fix in: https://www.drupal.org/project/views_ef_fieldset/releases/8.x-1.7 →

Hi @deborahblessy,

Thanks for helping with the issue.

There is already an issue fork and merge request in this issue that would conflict with the changes you propose. If you would like to contribute to this issue I would suggest you co-ordinate with @andy-blum in the #Olivero channel in Drupal slack to avoid working at cross purposes.

@wim-leers I think this still should be postponed, because this MR adds validation to the core config type of colour-hex.

That is used in two different places with different logic. Therefore we can't match the expectations of the logic until they are synchronised in ✨ Allow three character hex colour definitions in Olivero settings Active .

If we proceeded as-is with this change, config that used three character codes in image style configuration would suddenly become invalid.

I'm assuming as this issue is 10 months old that either a way forward has been found, or is no longer require.

Please feel free to re-open if you still need support to resolve.

Have created ✨ Allow three character hex colour definitions in Olivero settings Active to discuss making Olivero consistent with Image module's colour validation.

Postponing this issue based on the outcome of that one.

Eli-T → created an issue.

The test is asserting that a set of modules are enabled, and is failing because block_content is not enabled. However, debugging shows it would fail on other modules - here's a list of the enabled status of all checked modules.

action = true
announcements_feed = true
automated_cron = true
ban = true
basic_auth = true
big_pipe = true
block = true
block_content = false
book = false
breakpoint = false
ckeditor5 = false
comment = false
config_translation = false
contact = false
content_moderation = false
content_translation = false
contextual = false
datetime = false
datetime_range = false
dblog = false
dynamic_page_cache = true
editor = false
field = false
field_layout = false
field_ui = false
file = false
filter = true
forum = false
help = true
history = false
image = false
inline_form_errors = true
jsonapi = false
language = false
layout_builder = false
layout_discovery = false
link = false
locale = false
media = false
media_library = false
menu_link_content = false
menu_ui = false
migrate = false
migrate_drupal = false
migrate_drupal_ui = false
node = false
options = false
page_cache = true
path = false
pgsql = true
phpass = false
responsive_image = false
rest = false
sdc = false
search = false
serialization = false
settings_tray = false
shortcut = false
sqlite = true
statistics = false
syslog = true
experimental_module_requirements_test = true
experimental_module_test = true
system_test = true
taxonomy = false
telephone = false
text = false
toolbar = false
tour = true
update = true
views = false
views_ui = false
workflows = false
workspaces = false

The assertion that is currently failing is in ConfigImportAllTest::testInstallUninstall() on line 147

    // Check that all modules that were uninstalled are now reinstalled.
    $this->assertModules(array_keys($modules_to_uninstall), TRUE);

Eli-T → made their first commit to this issue’s fork.

This MR introduces a new Constraint that checks that an entity with a given ID and entity type exists. This is probably useful in a number of other places so it would be worth reviewing issues that might need it and linking so we're not duplicating work

Eli-T → made their first commit to this issue’s fork.