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

Resolving Issue With Named Parameters and Dispatcher #16694

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

Fenikkusu
Copy link
Contributor

@Fenikkusu Fenikkusu commented Feb 6, 2025

Hello!

  • Type: bug fix
  • Link to issue: N/A

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

Small description of change:

Since PHP 8.0, call_user_func_array now uses arrays with non-numeric keys as named parameters. If you have an action that does not use/define properties, this causes the error Unknown named parameter. While this can be avoided by adding the parameters, this change fixes it so this is no longer an issue. It also adds in the ability to hook into an event for the calling of the handler, though that was originally just confirmation of the bug. Just making the change to call array_values is enough otherwise.

Thanks

@Fenikkusu Fenikkusu changed the title Resolving Issue With Named Parameters and Dispatcher [WIP] Resolving Issue With Named Parameters and Dispatcher Feb 6, 2025
@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch 3 times, most recently from b6b7aed to 0a3dc09 Compare February 6, 2025 13:42
@niden
Copy link
Member

niden commented Feb 6, 2025

@Fenikkusu I will look at this a bit later on. Can you please rebase to target the 5.0.0 branch? We don't merge to master other than releases.

Thanks

@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch from a9656a9 to 224eba8 Compare February 6, 2025 15:36
@Fenikkusu Fenikkusu changed the base branch from master to 5.0.x February 6, 2025 16:52
@Fenikkusu
Copy link
Contributor Author

@Fenikkusu I will look at this a bit later on. Can you please rebase to target the 5.0.0 branch? We don't merge to master other than releases.

Thanks

I was wondering about that. Since it was 5.8.0, I didn't see a compatible branch, hence I went with master. I've updated it to 5.0.x.

@Fenikkusu
Copy link
Contributor Author

...And Now I've got to do a full rebase...fudge...

@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch 2 times, most recently from 78c1154 to 7b2dceb Compare February 6, 2025 17:01
@niden
Copy link
Member

niden commented Feb 6, 2025

@Fenikkusu don't worry about the PHP 8.4 build failing (from PECL). That is what we are working on now.

@niden
Copy link
Member

niden commented Feb 6, 2025

I mean leave the header files as they are. If your PR only fails on PHP 8.4 build then it is good enough.

@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch from 46b3ad5 to 46b519c Compare February 6, 2025 17:20
@Fenikkusu
Copy link
Contributor Author

I mean leave the header files as they are. If your PR only fails on PHP 8.4 build then it is good enough.

I've reverted them out. I get anal about making sure all the testing is passing.

@Fenikkusu Fenikkusu changed the title [WIP] Resolving Issue With Named Parameters and Dispatcher Resolving Issue With Named Parameters and Dispatcher Feb 6, 2025
@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch 5 times, most recently from b94fd55 to 3bd99a6 Compare February 6, 2025 19:06
phalcon/Dispatcher/AbstractDispatcher.zep Outdated Show resolved Hide resolved
@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch 4 times, most recently from 27322c0 to cbc5960 Compare February 7, 2025 03:20
@Fenikkusu
Copy link
Contributor Author

Fenikkusu commented Feb 7, 2025

  • Cleaned & Squashed Commits
  • Added Testing For No Params, With Named Params, and Events
  • Updated Change Log

Side Thought: I normally use Route Converters to Convert IDs to Models. One downside of doing it this way is that you cannot throw proper errors and have them redirected as you can with traditional dispatcher events. In theory, someone could use the new events to do this instead of using Converters.

@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch from cbc5960 to df7a770 Compare February 7, 2025 03:24
Updating Changelog
Adding Unit Test
@Fenikkusu Fenikkusu force-pushed the hotfix/controller-named-params branch from df7a770 to 8bee72a Compare February 7, 2025 03:25
@niden niden merged commit 4d1b750 into phalcon:5.0.x Feb 7, 2025
42 of 43 checks passed
@niden
Copy link
Member

niden commented Feb 7, 2025

Thank you @Fenikkusu

@Fenikkusu Fenikkusu deleted the hotfix/controller-named-params branch February 9, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants