Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Order NodeTypes found inside NodeTypes folder #4394

Closed
wants to merge 2 commits into from

Conversation

mhsdesign
Copy link
Member

Placing NodeTypes into the NodeTypes folder will currently yield a non-deterministic order when they are scanned.

One can observe this when running flow configuration:show --type NodeTypes.
The order might change from installation to installation with the same sources.
Also you can observe that with this fix, the order is influenced by the file name.

Thats because the underling directory scan via the RecursiveDirectoryIterator doesn't enforce a specific order, but will return the files how the underlying file system returns them - on linux - unordered.

The NodeTypes configuration folder was introduces with #3332.

This was first noticed, when the nodeTypes would show up in a different order in the "Create New" dialog in the Neos UI:

neos/neos-ui#3560 (comment)

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

 Placing NodeTypes into the `NodeTypes` folder will currently yield a non-deterministic order when they are scanned.

 One can observe this when running `flow configuration:show --type NodeTypes`.
 The order might change from installation to installation with the same sources.
 Also you can observe that with this fix, the order is influenced by the file name.

 Thats because the underling directory scan via the `RecursiveDirectoryIterator` doesn't enforce a specific order, but will return the files how the underlying file system returns them - on linux - unordered.

 The `NodeTypes` configuration folder was introduces with #3332.

 This was first noticed, when the nodeTypes would show up in a different order in the "Create New" dialog in the Neos UI:

 neos/neos-ui#3560 (comment)
@github-actions github-actions bot added the 7.3 label Jul 9, 2023
mhsdesign added a commit to grebaldi/PackageFactory.Guevara that referenced this pull request Jul 9, 2023
There is specificity problem. `Opens Above` appears in both nodeType labels, and thus both might be clicked but testcafe decides for the first occurrence. And the nodeTypes order is currently non deterministic neos/neos-development-collection#4394
mhsdesign added a commit to grebaldi/PackageFactory.Guevara that referenced this pull request Jul 9, 2023
There is specificity problem. `Opens Above` appears in both nodeType labels, and thus both might be clicked but testcafe decides for the first occurrence. And the nodeTypes order is currently non deterministic neos/neos-development-collection#4394
Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

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

LGTM by reading

@mhsdesign
Copy link
Member Author

mhsdesign commented Jul 10, 2023

@gjwnc posted some useful insights (thank you ❤️) about the unsortiness in other places as well, and that even using the old NodeTypes loading it will be unsorted. This questions this fix as it might not be worth and intended to be fixed.

———

None of the configs is or was sorted. Not the nodeTypes or Settings or Policy. Before neos/neos-development-collection#3332, the config was mainly loaded using Flow's YamlSource::load. This one is using glob() which, regarding sorting, has the same behavior.
Regarding Settings or other yaml files you just often don't notice the difference between your various installations. This is because your package order introduces a "good enough" hierarchy which, after merging the yaml files, results in, apart from sorting, the same yaml config - e.g. you likely find the same values using the same path injecting a setting.
A very simple example to show the unsortedness can be ./flow configuration:show itself. If you have many packages, it is likely that there is a difference in the results from your local dev and production. I don't mean a difference of existing/not existing packages because of require-dev, I mean that the order of settings is just different also for packages existing on both installations.

A quick check on a project of ours showed, that on production I get

PunktDe:
    EditConflictPrevention:
...
Sandstorm:
    TemplateMailer:

and on my local dev it is reversed - Sandstorm before PunktDe.

@gjwnc
Copy link
Contributor

gjwnc commented Jul 11, 2023

This questions this fix as it might not be worth and intended to be fixed.

If I could decide, I'd vote for your PR 🤞 I didn't want to discourage you with my comment. It was meant as showing a bigger picture. Regarding configuration I think it would be great to have consistent and predictable results.
I would also go one step further and sort the packages and loaded yaml source, i.e.

  1. Add \ksort($this->unsortedPackages); to PackageResolver
  2. Add \sort($pathsAndFilenames) to YamlSource

This should result in predictable results for all installations and all configs. 😃 But yeah, not sure if there is any interest in such little changes likely no one will even recognize, thus maybe not much of a need 🙈 But I still favor predictability because it avoids future confusion.

@dlubitz
Copy link
Contributor

dlubitz commented Jul 11, 2023

Not sure if this is really an issue. The unsorted order can only happen within a package OR cross-package if no dependency is defined.

Within a package the developer has to ensure, that there are no concurrent configurations, which could lead to different results.

Cross-package you have to define dependencies anyways, as otherthings (e.g. Fusion) will break too.

BUT I would expect a loading order of the files by name within a package, too.

@kdambekalns
Copy link
Member

Basically all was said:

  • sorting never happened on name so far
  • within a package you should not need to rely on sorting by name
  • packages are generally sorted by dependencies, must not be changed

Adding one thing I can think of:

  • it might be good to have packages with same "priority" sorted by someting to be stable

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

See inline comment…

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

I'm skeptical with this change (as written on Slack).

The new behavior is fine of course, but it adds complexity that is not needed IMO because the order of node types depend on the usecase.
E.g. in the UI we should mostly order them according to nodeType.ui.position – in other parts you might want to show document node types first etc..

@mhsdesign
Copy link
Member Author

Yes indeed for the ui they are sorted via the position key. @ahaeslich also mentioned that the ui should resort as fallback to sorting them by name and it seems also be declared that way:

https://github.com/neos/neos-ui/blob/c2a7e0867d2c9fb26b75d4f9bbb82d75364633ec/packages/neos-ui-contentrepository/src/registry/NodeTypesRegistry.ts#L137C82-L137C82

but the sorting doesnt seem to take effect.

See neos/neos-ui#3560 (comment)

@mhsdesign
Copy link
Member Author

I'm skeptical with this change
[...] it adds complexity that is not needed IMO

i agree and i opened up a flow issue where we can discuss if we want to fix it all or keep everything as is:
neos/flow-development-collection#3300

It does not make sense to first fix this problem on the outmost leaf in this neos config loader. Thats why ill be closing this pr.

@mhsdesign mhsdesign closed this Feb 1, 2024
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.

5 participants