flow-development-collection icon indicating copy to clipboard operation
flow-development-collection copied to clipboard

TASK: Declare state properties in controller to be readonly

Open mhsdesign opened this issue 1 year ago • 0 comments

In https://github.com/neos/flow-development-collection/pull/3232#discussion_r1467443443 i mentioned that "replacing" the $this->response is probably not a good idea and we should disallow this via readonly. Mutating by object reference is the only way allowed.

I tried to actually enforce this via real readonly in https://github.com/neos/flow-development-collection/pull/3283, but it has some severe flaws:

  • There was only one place where we ourselves replace the variable for real, but i solved it with a mergeIntoParent or return.
  • that means that controllers cant be singletons anymore (why are they declared as such even in general sometimes???)
    • the only usecase of a controller being a singleton would be for forwarded requests, but that is hacky as hell to then rely on internal state :D
  • In widgets we rely on that the controller's process to be called multiple times: https://github.com/neos/flow-development-collection/blob/465c80be0b6753356fe6e2c71178405970bf1fea/Neos.FluidAdaptor/Classes/Core/Widget/AbstractWidgetViewHelper.php#L241 which is also not forbidden per se.

As compromise i suggest to use the read only annotation to make people aware.

Upgrade instructions

You should never replace any of the stateful properties of the action controller.

Review instructions

This pr should not be merged before targeting 9.0. See https://github.com/neos/flow-development-collection/pull/3232#issuecomment-1915676748 for explanation.

TODO

  • [ ] Can the ignore readonly be by any chance breaking?? like use Neos/Flow/Annotations/Inject as readonly

mhsdesign avatar Jan 29 '24 23:01 mhsdesign