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.