Skip to content

Conversation

@mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Feb 17, 2025

While cleaning up code in the reflection service #3447 i noticed the sorting calls and suspected that they have become obsolete over the time. Especially as the reflection was refactored now and that some explanations seem dated.

after reflecting each class we sort classReflectionData which can be avoided
but we still ensure that getAllClassNames returns all classes sorted.
For using array_diff to get the $newClassNames we dont need sorting

Locally removing the sort calls is just fine but the CI throws this exception.

There was 1 failure:

1) Neos\Flow\Tests\Functional\Reflection\ReflectionServiceTest::aggregateRootAssignmentsInHierarchiesAreCorrect
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Neos\Flow\Tests\Functional\Reflection\Fixtures\Repository\SubSubEntityRepository'
+'Neos\Flow\Tests\Functional\Reflection\Fixtures\Repository\SuperEntityRepository'

/home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/Packages/Framework/Neos.Flow/Tests/Functional/Reflection/ReflectionServiceTest.php:90
/home/runner/work/flow-development-collection/flow-development-collection/flow-development-distribution/bin/phpunit:122

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@github-actions github-actions bot added the 9.0 label Feb 17, 2025
@mhsdesign mhsdesign force-pushed the task/followup-3443-code-style-run-tests branch from bca7a7c to 5ed4e86 Compare February 17, 2025 21:11
after reflecting each class we sort classReflectionData which can also not be done instead :D

but we still ensure that getAllClassNames returns all classes sorted.

For using `array_diff` to get the $newClassNames we dont need sorting
@mhsdesign mhsdesign force-pushed the task/followup-3443-code-style-run-tests branch from 5ed4e86 to cb5de61 Compare February 18, 2025 07:55

return array_keys($this->classReflectionData);
$classNames = array_keys($this->classReflectionData);
sort($classNames);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as karsten wrote

Why the sorting? Is that the central replacement for the removed sort calls later on?

yes as the structure is not internally sorted anymore but getAllClassNames guaranteed A-Z order before i thought we should continue to do so.
On the other hand, getAllClassNames can be considered rather low level and is only used once so we can also put sorting on the callers site - maybe no sorting is needed for every usecase.

@mhsdesign mhsdesign mentioned this pull request Feb 18, 2025
6 tasks
@mhsdesign mhsdesign changed the title Task/followup 3443 code style run tests TASK: Remove obsolete sorting of big arrays in reflection service Feb 18, 2025
@github-actions github-actions bot added the Task label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant