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

EXPERIMENT: FEATURE: Introduce trait ActionToMethodDelegation to create own simple controller. #3297

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

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jan 30, 2024

The name of the previously suggested SimpleActionController looked for improvement.

The Simple* prefix suggest, that the initial implementation blew up too much. But ideally a name would be more obvious and not suggest that there is another complex action controller.

I could not come up with a better name, but we can make the controllers more composable.

This change introduces a trait which implements the logic of the previous SimpleActionController.
That way people can implement the ControllerInterface directly and use a trait that will map action to method name.

 class SimpleActionTestController implements ControllerInterface
 {
     use ActionToMethodDelegation;

     public function addTestContentAction(ActionRequest $actionRequest): ActionResponse
     {
         $response = new ActionResponse();
         $response->setContent('Simple');
         return $response;
     }
 }

Upgrade instructions

Review instructions

See also

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

@kitsunet
Copy link
Member

kitsunet commented Jan 30, 2024

Haha, well, I wanted to do this after I finish my work stuff, well, good good. Btw, this ignores the public method thing you noted on the original simple controller chnage before, and I wanted to comment that that needs reflection which is nasty, so I would rather not check visibility.

@mhsdesign mhsdesign changed the title TASK: Introduce trait ActionToMethodDelegation instead of SimpleActionController. FEATURE: Introduce trait ActionToMethodDelegation instead of SimpleActionController. Jan 30, 2024
@mhsdesign
Copy link
Member Author

I was unsure to do this in this pr or let this trait first be destroyed by someone who dislikes it :D

Actually i did plan on using reflection as its super fast nowadays (faster than property mapping :D)

https://stackoverflow.com/questions/4160901/how-to-check-if-a-function-is-public-or-protected-in-php

Is Callable way: 0.0033209323883057ms
Reflection way: 0.0052220821380615ms

@mhsdesign
Copy link
Member Author

Aaand i really think that we should keep this change out of the main pr #3232 (comment) right now (also the class SimpleActionController) as this might otherwise get 90% of the discussion focus and the rest of that big pr is lost.

The introduction of process(): Response might be enough ^^ for one pr ^^

@kitsunet
Copy link
Member

I was unsure to do this in this pr or let this trait first be destroyed by someone who dislikes it :D

Actually i did plan on using reflection as its super fast nowadays (faster than property mapping :D)

https://stackoverflow.com/questions/4160901/how-to-check-if-a-function-is-public-or-protected-in-php

Neat, well then, great, we can check callable and public in one go.

@kitsunet
Copy link
Member

We should/could use this also for the existing Controller?!

@mhsdesign
Copy link
Member Author

We should/could use this also for the existing Controller?!

Yes sounds great. And im already glad this is an own pr 😅

@mhsdesign mhsdesign changed the title FEATURE: Introduce trait ActionToMethodDelegation instead of SimpleActionController. FEATURE: Introduce trait ActionToMethodDelegation to create own simple controller. Jan 30, 2024
mhsdesign added a commit to kitsunet/flow-development-collection that referenced this pull request Feb 3, 2024
@mhsdesign
Copy link
Member Author

We also discussed to keep the *Action pattern or not: #3232 (comment) but it seems its here to stay :)

do we want to keep the *Action pattern, because it still happens to me that i miss the suffix and it takes some time to figure out again what i missed 😅 , to fix this we could just improve the routing:match command or the like to emit a warning, or wait we cannot as this is highly dynamic and we have to have an actual request at hand.

Mmm, yeah I thought about changing it, and I wouldn't mind that, for me I would actually say just make it an attribute? just allowing any method (without marker like an attribute) seems dangerous-ish especially if someone doesn't do proper sec policies. Also this then wouldn't match any existing policies as they all assume Action, so I just kept it that way.

@mhsdesign mhsdesign force-pushed the temporary-mvc-refactoring-target-branch branch from 57170ee to f5b137f Compare February 3, 2024 13:08
…tionController`.

The name of the previously suggested `SimpleActionController` looked for improvement.

The `Simple*` prefix suggest, that the initial implementation blew up too much. But ideally a name would be more obvious and not suggest that there is another _complex_ action controller.

 I could not come up with a better name, but we can make the controllers more composable.

 This change introduces a trait which implements the logic of the previous `SimpleActionController`.
 That way people can implement the `ControllerInterface` directly and use a trait that will map action to method name.
@mhsdesign mhsdesign force-pushed the feature/ActionToMethodDelegationTrait branch from b9e1b79 to cadaaba Compare February 3, 2024 13:14
@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
@mhsdesign mhsdesign changed the title FEATURE: Introduce trait ActionToMethodDelegation to create own simple controller. EXPERIMENT: FEATURE: Introduce trait ActionToMethodDelegation to create own simple controller. Apr 24, 2024
neos-bot pushed a commit to neos/flow that referenced this pull request Sep 12, 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.

2 participants