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

TASK: Declare state properties in controller to be readonly #3293

Draft
wants to merge 4 commits into
base: temporary-mvc-refactoring-target-branch
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jan 29, 2024

In #3232 (comment) 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 #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:
    $subResponse = $this->controller->processRequest($subRequest);
    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 #3232 (comment) for explanation.

TODO

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

@mhsdesign mhsdesign force-pushed the task/declareStatePropertiesInControllerToBeReadonly branch from 2962d8b to 026ff6f Compare February 3, 2024 14:51
@mhsdesign mhsdesign force-pushed the temporary-mvc-refactoring-target-branch branch 2 times, most recently from 080c854 to f37ce2c Compare March 3, 2024 09:15
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