🇬🇧United Kingdom @daniel.j

I've added `MenuDataExtensionTest` to add coverage for this functionality.

Otherwise, I think that this is ready to go, unless it would be good to add another way to specify the shape / size of the menu data other than just the array shape that is implemented already?

Also, would it be good to include any other keys in the menu array, or is the super basic "title", "URL", "attributes" and "below" good enough?

daniel.j → created an issue.

Rebased onto 11.x, mostly taking the changes from 11.x including the SQL statements. Also run run the tests and adjusted the metrics as they were failing after the rebase. Removed the uses of the depricated CacheTag functions.

Unsure if we should proceed with this or the child issues over there.

I don't think I'm the right person to answer this, but this work does include some tests that were not added as part of the other duplicate issue, namely limited testing for cold and cool caches as well as listing out in full the expected 'CacheTagGroupedLookups' for several tests. Are these useful additions?

Either way, the MR should be up-to-date and mergable now. Even if it's not merged it was still a good way to get more familiar with Gander. Thanks!

daniel.j → changed the visibility of the branch 3451080-d11_ready to hidden.

Hey, I've added performance assertions in 'OpenTelemetryFrontPagePerformanceTest' and 'OpenTelemetryNodePagePerformanceTest'.

I started by asserting exact values and correcting them until the tests passed. Where performance metrics varied on each run, mostly with the cold cache tests, they were moved to using 'assertCountBetween', gradually expanding the acceptable range until the tests passed reliably.

The exact database query strings have not been included (like they are in 'StandardPerformanceTest') as the query counts are either 0 of 100+, which seems like a lot to have hardcoded. However, the exact 'CacheTagGroupedLookups' have been included where the values are reliably the same across test runs.

Additionally, this work introduced the word 'languageswitcher' which failed with 'cspell'. I can see that elsewhere this has been marked 'cspell:ingore'. Instead I've added 'languageswitcher' to the 'core/misc/cspell/dictionary.txt'. This looked like the best place to put it. Is this correct? Would it be good to remove the 'cspell:ignore' with this change?

Are there any additional performance assertions that would be good to include? Are the ones added suitable?

There seem to be some JavaScript tests failing, but these look to be unrelated to this work. I've triggered the performance test on the MR, but as of writing this job is stuck (https://git.drupalcode.org/issue/drupal-3408713/-/jobs/4488014).

It would be great for this work to be reviewed! Thanks!

daniel.j → changed the visibility of the branch 11.x to hidden.

Rebased onto 11.x and resolved conflicts.

Please review again, thanks!

Added DRUPAL_CORE variable to the Gitlab CI composer build variables and added a .cspell-project-words.txt.

Pipelines are now passing, please review. Thanks!

Added a .eslintrc.yml file to ignore import and no-console eslint errors specifically in the gulpfile.js. Changes also include anonymous function and eslint automatic fixes.

Pipelines are now passing, please review. Thanks!

I've made some changes and tests are now passing.

For the phpunit functional JS tests, in some places moving away from jQuery to native JS has allowed the tests to pass. Additionally, I've added a `composer.libaraies.json` file in order to add clarity to the library versions that are being used.

Please review, thanks.

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

Added method return types and removed corresponding ignored errors from `.phpstan-baseline.php`.

The remaning ignored errors for the navigation module in `.phpstan-baseline.php` relate to methods included in the navigation module via traits from other modules. I've only addressed changed to the navigation module strictly, so have left these ignored errors for now.

Please see the MR, a review would be great. Thanks!

daniel.j → changed the visibility of the branch 3480135-enforce-return-types to active.

daniel.j → changed the visibility of the branch 3480135-enforce-return-types to hidden.

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

Added phpcs and phpstan fixes. However, the composer job in the gitlab-ci pipeline is installing drupal core 11 which conflicts with commerce 2.

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

Updated the issue summary, adding more information to the problem summary and a proposed resolution.

Hopefully this is suitable for this issue, please review. Thanks!

Added a .gitlab-ci.yml file and implemented phpstan, phpcs and cspell fixes.

Gitlab CI pipeline passing (https://git.drupalcode.org/issue/mollie_donations-3458108/-/pipelines/32...).

Please review, thanks.

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

Added .gitlab-ci.yml file to allow tests to be run.

Please review, thanks

daniel.j → created an issue.

@catch I've changed the return comment to reflect your suggestion. Please review, thanks

Phpcs answered my question regarding the '??'.

Re-rolled patch to target 3.0.x-dev. I've not changed the use of the inline if statement in favour of '??'. Assuming that we no longer need to support php5 though, can instead use '??' for more concise code.

Is this change still desired? Thanks

MR openned and fixes included for:
- phpstan
- phpcs
- stylelint
- eslint
- cspell

There are some fixes still required for eslint mostly related to module imports and loops (https://git.drupalcode.org/issue/dynamic_layouts-3458110/-/jobs/3210629).

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

Changed the Phpdoc to better reflect the usage and return value of the 'fetchPluginNames' method in 'core/modules/views/src/Views.php'.

I've openned a merge request for the change. A review would be great, thanks.

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

Thanks! Now a new merge request targeting 3.0.x, hopfully this looks better.

daniel.j → changed the visibility of the branch 3.0.x to hidden.

Added the comment "Invalid IDs are not included in the returned array" to loadMultiple documentation in core/lib/Drupal/Core/Entity/EntityStorageInterface.php.

I've openned a MR with the change, it would be great if this could be reviewed. Thanks.

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

@jsacksick Apologies if I've misunderstood, I mean the addition to of the class docs rather than the commerce documentation.

I've openned a MR for review (https://git.drupalcode.org/project/commerce/-/merge_requests/357), however the composer step of the pipeline is failing.

Just encountered patch #14 not applying when upgrading to the latest version of admin_toolbar.

I've updated the fork with upstream changes, re-rolled the patch and openned a MR.

I can confirm that the patch in #14 no longer applies on 3.5.0 but I'm also no longer seeing the full menu flash issue.

I have not been able to confirm that the issue is still present or not. Has any one else experienced the issue recently / can replicate it one the latest version?

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

This means that creating and saving an entity through the API, as you would in a test, doesn't add trait fields

Looking back at the issue description, I think that the issue is still valid. However, the postSave method of EntityDuplicateFormTrait (which dispatches an event listened to by EntityDuplicateSubscriber) would need to not re-add fields that have been removed in the entity save or postSave as part of the removal of an EntityTrait.

Having a further look, the fields from the source entity are coppied accross to the cloned entity in the 'postSave' of 'EntityDuplicateFormTrait', which is called after the entity save.

Even if in the entity save we handle the added / removed traits' fields, they are added again afterward in the 'SourceTypeForm'. I think that this is why the trait logic is handled at the form level, so that it can be the last word after both the entity saves and 'EntityDuplicateFormTrait'.

I think that there would have been value in making it clearer as to why it's done in this order. Do you think that it would be good to add a comment to explain why it has been done this way?

Considering the small interest expressed with this, wondering if we should really spend time "fixing" this.

Personally, I've been looking at this issue as part of my efforts to start getting into making contributions, so I've been spending time on it exactly because it's not high priority :)

Moving the logic to the 'postSave' has its own issues due to the way that the 'selected_traits' and 'unselected_traits' are calculated.

    $selected_traits = array_diff($traits, $original_traits);
    $unselected_traits = array_diff($original_traits, $traits);

With this inside of the 'postSave', there is no longer visability of the original traits on the entity (unless I'm missing something) in order to work out what traits have been added and removed as the new selection has already been saved to the entity.

The test that is failing is the 'testDuplicateTraits' test of 'Drupal\Tests\commerce\Functional\EntityTraitTest'.

When an entity type is cloned from an entity type with an EntityTrait that provides a field, the test checks that by removing the EntityTrait and saving the new cloned entity type the provided field is also removed. As the entity being saved returns TRUE when it's 'isNew()' method is called, the save logic assumes that there are no original traits.

    if ($this->isNew()) {
      $original_traits = [];
    } else {
      $original = $this->entityTypeManager()->getStorage($this->getEntityTypeId())->loadUnchanged($this->id());
      $original_traits = $original->getTraits();
    }

When the trait handling logic was handled at the form level, it was called after the entity had been saved, so at that point the entity was no longer new and the original traits from the cloned entity were loaded.

I feel like we should either:
A) Not go ahead with this change now that we know why the traits are handled at the form level and not the API level. Perhaps adding a comment to explain why.
B) Not assume that just because an entity is new it has no traits as this does not account for the situation where the new entity has been cloned from an entity that has traits.

I hope this is helpful and not poorly explained. I'll be interested to hear your thoughts.

Merge request created

I've added this line of documentation to each of the other services in the commerce core and sub-module services in the '2914933-document-service-tags-service-interfaces' branch, but not made a MR.

It would be great if this could be reviewed. Is this change still desired? Should it be done in a different way? Thanks!

I've created a fork and branch for this work and pushed changes inline with what jochim suggested.

Currently, I've only added to the docs of 'service_collector' tags in the 'commerce_order' module. I plan to continue to add this documentation in this style to the other modules and declared services.

Please let me know if this change is no longer desired / if a different approach would be better.

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

Had a look at this issue as part of getting into contributing.

Attached is a new patch, hopfully this was the intention of a cleanup. The `submitTraitForm` from `CommerceBundleEntityFormBase.php` has been removed, as well as the calls in the extending Forms.