Allow typed finder arguments by othercorey · Pull Request #16782 · cakephp/cakephp

othercorey · 2022-10-01T07:48:39Z

refs #12787

~~This does not remove the $options parameter. Users must specify an empty array if they want to by-pass it.~~

The $options parameter is now optional.

othercorey · 2022-10-01T07:52:08Z

@robertpustulka I remember arguing about this back when I knew nothing. I hope this looks reasonable.

tests/TestCase/ORM/TableTest.php

othercorey · 2022-10-09T09:32:38Z

src/ORM/Association.php

+                /** @psalm-suppress ReferenceReusedFromConfusingScope */
+                $options = $options + $opts;
+            } else {
+                array_unshift($args, $opts);


Users will receive a PHP error if they don't have a proper array parameter for options. I can't create exceptions for all edge cases leading to this in all finders so going to rely on PHP errors to tell users their finder can't support options.

othercorey · 2022-10-09T09:33:57Z

src/ORM/BehaviorRegistry.php

+                $reflected = new ReflectionFunction($callable);
+                $param = $reflected->getParameters()[1] ?? null;
+                if ($param?->name === 'options' && !$param->isDefaultValueAvailable()) {
+                    $args = [[]];


This adds an empty options array when calling find with no arguments and the finder expects $options.

An example of this is Table::findAll() and calling it with ->find().

It's quite unfortunate that the $options arg doesn't default to an empty array currently. It would have made no practical difference.

Perhaps we can add the default value now to core finders and throw a warning when this if returns true so that we can at least remove this reflection in the future.

Perhaps we can add the default value now to core finders and throw a warning when this if returns true so that we can at least remove this reflection in the future.

This sounds like a great inclusion into 4.5.0. We could do the detection and deprecation warning there, and in 5.x we can enforce a default options value, or we can start spreading options as named parameters.

I think the point I want to make, is that if we're going to break compatibility why not do what would be best going forward, and then we can figure out how we add deprecations and backporting to 4.5 to make the upgrade process understandable and incrementally adoptable.

src/ORM/Table.php

ADmad · 2022-10-09T12:05:05Z

src/ORM/BehaviorRegistry.php

+                $reflected = new ReflectionFunction($callable);
+                $param = $reflected->getParameters()[1] ?? null;
+                if ($param?->name === 'options' && !$param->isDefaultValueAvailable()) {
+                    $args = [[]];


It's quite unfortunate that the $options arg doesn't default to an empty array currently. It would have made no practical difference.

Perhaps we can add the default value now to core finders and throw a warning when this if returns true so that we can at least remove this reflection in the future.

src/ORM/Table.php

markstory · 2022-10-11T01:59:16Z

src/ORM/Association.php

+        if ($opts) {
+            if (is_array($options)) {
+                /** @psalm-suppress ReferenceReusedFromConfusingScope */
+                $options = $options + $opts;


Do we use options after this merge? Or are you relying on the ref from the previous block to mutate the options in-place? We should leave a comment for future us if that is the solution that is used here.

markstory · 2022-10-11T02:01:28Z

src/ORM/Association/BelongsToMany.php

+        $options = null;
+        if ($args) {
+            if (array_key_exists(0, $args)) {
+                $options = &$args[0];
+            } elseif (array_key_exists('options', $args)) {
+                $options = &$args['options'];
+            }
+        }
+
+        if ($opts) {
+            if (is_array($options)) {
+                /** @psalm-suppress ReferenceReusedFromConfusingScope */
+                $options = $options + $opts;
+            } else {
+                array_unshift($args, $opts);
+            }
+        }


Should there be a function for this? There are a few implementations of this in these changes. Having a function might let us contain the reference tricks into code into that is less frequently changed and easier to remove when the time comes.

markstory · 2022-10-11T02:10:02Z

src/ORM/BehaviorRegistry.php

+                $reflected = new ReflectionFunction($callable);
+                $param = $reflected->getParameters()[1] ?? null;
+                if ($param?->name === 'options' && !$param->isDefaultValueAvailable()) {
+                    $args = [[]];


Perhaps we can add the default value now to core finders and throw a warning when this if returns true so that we can at least remove this reflection in the future.

This sounds like a great inclusion into 4.5.0. We could do the detection and deprecation warning there, and in 5.x we can enforce a default options value, or we can start spreading options as named parameters.

I think the point I want to make, is that if we're going to break compatibility why not do what would be best going forward, and then we can figure out how we add deprecations and backporting to 4.5 to make the upgrade process understandable and incrementally adoptable.

markstory · 2022-10-11T02:15:43Z

src/ORM/Table.php

+     * Here, the finder "findByCategory" does have an `$options` parameter:
+     *
+     * ```
+     * function findByCategory(SelectQuery $query, array $options, int $category): SelectQuery


In the spirit of doing what is best going forward, I would like to propose we drop support for $options entirely. Instead we only support named parameters in 5.x. This is a meaningful break in backwards compatibility, but I think it will provide a long term improvement in developer experience. With named parameters we get typed checked optional parameter support. It's everything we ever wanted from $options.

I feel like deprecating $options in 4.5 is something we can do. Users that would want to upgrade to 5.x will need to upgrade PHP before they can use named parameters. I don't feel bad about this as encouraging them to upgrade PHP will be something they would need to do anyways.

Making $options options is definitely complicated. It looks like code we don't want to support.

Are you suggesting we drop $options and force users to call applyOptions() directly? I'm a little confused when you say drop it but then say support it with named parameters.

I would be okay with dropping $options if we retain a way to still be able to pass values for the query clauses to the find() call. Something like find('foo', select: $a, order: $b, custom: 'bar').

We could do that. All of the query-specific options would then become reserved keywords for parameter names.

markstory · 2022-10-11T02:16:55Z

src/ORM/Table.php

+                $query->applyOptions($options);
+                /** @psalm-suppress ReferenceReusedFromConfusingScope */
+                $options = $query->getOptions();


We should do this after the options juggling function is completed.

othercorey · 2022-10-13T05:34:56Z

We don't want to commit this optional $options implementation. Will look for alternatives.

othercorey added enhancement ORM labels Oct 1, 2022

othercorey added this to the 4.5.0 milestone Oct 1, 2022

othercorey force-pushed the finder-arguments branch 2 times, most recently from db87083 to e8a8e13 Compare October 1, 2022 08:16

ADmad reviewed Oct 1, 2022

View reviewed changes

tests/TestCase/ORM/TableTest.php Outdated Show resolved Hide resolved

othercorey force-pushed the finder-arguments branch from e8a8e13 to f69662c Compare October 9, 2022 05:32

othercorey changed the title ~~Allow typed finder arguments after the $options array~~ Allow typed finder arguments Oct 9, 2022

othercorey requested review from ADmad and markstory October 9, 2022 05:32

othercorey force-pushed the finder-arguments branch 5 times, most recently from 6d5903b to 7c78e5b Compare October 9, 2022 09:31

othercorey commented Oct 9, 2022

View reviewed changes

othercorey force-pushed the finder-arguments branch from 7c78e5b to 4c98c21 Compare October 9, 2022 10:24

ADmad reviewed Oct 9, 2022

View reviewed changes

othercorey force-pushed the finder-arguments branch from 4c98c21 to 23635af Compare October 9, 2022 13:50

ADmad reviewed Oct 9, 2022

View reviewed changes

src/ORM/Table.php Outdated Show resolved Hide resolved

Allow typed finder arguments with or without an $options parameter

bd790ac

othercorey force-pushed the finder-arguments branch from 23635af to bd790ac Compare October 9, 2022 16:30

markstory reviewed Oct 11, 2022

View reviewed changes

othercorey closed this Oct 13, 2022

othercorey deleted the finder-arguments branch October 13, 2022 05:35

ADmad mentioned this pull request Apr 15, 2023

5.x finder arguments no options #17108

Merged

Conversation

othercorey commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

othercorey commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

othercorey Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ADmad Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

othercorey commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

othercorey commented Oct 1, 2022 •

edited

Loading

othercorey commented Oct 1, 2022 •

edited

Loading

othercorey Oct 9, 2022 •

edited

Loading

ADmad Oct 11, 2022 •

edited

Loading