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

Parameter name binding #87

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Parameter name binding #87

wants to merge 11 commits into from

Conversation

xepozz
Copy link
Member

@xepozz xepozz commented Feb 13, 2024

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues

Copy link

what-the-diff bot commented Feb 13, 2024

PR Summary

  • Introduction of Optional Parameter in isResolvable method
    An optional parameter has been added to the isResolvable method in the DefinitionStorage.php file to give developers the flexibility to decide whether or not to include certain data in the definition.

  • Enhance Checks for New Parameter in isResolvable method
    Additional conditions have been injected into the isResolvable method to establish whether the new parameter fits with the existing definition, improving the system's adaptability to new data.

  • Existing Definition Confirmation in isResolvable method
    The system will now confirm if a definition already resides within the definitions array before adding it. This will prevent any data redundancy and optimize memory utilisation.

  • Condition Addition for Definitions in normalize method
    In the normalize method in Normalizer.php, a new condition has been added that promptly returns the definition, should it correspond with an instance of DefinitionInterface. This expedites the process while accurately filtering required definitions.

@xepozz
Copy link
Member Author

xepozz commented Feb 13, 2024

Also it could be done as the patch does:

Index: src/ArrayDefinition.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/ArrayDefinition.php b/src/ArrayDefinition.php
--- a/src/ArrayDefinition.php	(revision 7ea68b63f82f218656c9aca735d41b4b41f8a8da)
+++ b/src/ArrayDefinition.php	(date 1707842233005)
@@ -7,12 +7,14 @@
 use InvalidArgumentException;
 use Psr\Container\ContainerInterface;
 use ReflectionMethod;
+use ReflectionNamedType;
 use Yiisoft\Definitions\Contract\DefinitionInterface;
 use Yiisoft\Definitions\Contract\ReferenceInterface;
 use Yiisoft\Definitions\Exception\InvalidConfigException;
 use Yiisoft\Definitions\Helpers\ArrayDefinitionHelper;
 use Yiisoft\Definitions\Helpers\DefinitionExtractor;
 use Yiisoft\Definitions\Helpers\DefinitionResolver;
+use ReflectionUnionType;
 
 use function array_key_exists;
 use function call_user_func_array;
@@ -24,7 +26,7 @@
  *
  * @psalm-type MethodOrPropertyItem = array{0:string,1:string,2:mixed}
  * @psalm-type ArrayDefinitionConfig = array{class:class-string,'__construct()'?:array}&array<string, mixed>
- */
+*/
 final class ArrayDefinition implements DefinitionInterface
 {
     public const CLASS_NAME = 'class';
@@ -196,6 +198,27 @@
                 /** @infection-ignore-all Mutation don't change behaviour. Values of `$usedArguments` not used. */
                 $usedArguments[$index] = 1;
             }
+            if ($value instanceof ParameterDefinition) {
+                $type = $value->getReflection()->getType();
+                if ($type !== null) {
+                    if ($type instanceof ReflectionUnionType) {
+                        foreach ($type->getTypes() as $type) {
+                            $boundDefinition = $type->getName() . '$' . $key;
+                            if (isset($arguments[$boundDefinition])) {
+                                $value = DefinitionResolver::ensureResolvable($arguments[$boundDefinition]);
+                                $usedArguments[$boundDefinition] = 1;
+                                break;
+                            }
+                        }
+                    } elseif ($type instanceof ReflectionNamedType) {
+                        $boundDefinition = $type->getName() . '$' . $key;
+                        if (isset($arguments[$boundDefinition])) {
+                            $value = DefinitionResolver::ensureResolvable($arguments[$boundDefinition]);
+                            $usedArguments[$boundDefinition] = 1;
+                        }
+                    }
+                }
+            }
             $dependencyIndex++;
         }
         unset($value);

But still needed adjustments in the definition storage

src/DefinitionStorage.php Outdated Show resolved Hide resolved
@samdark samdark added the pr:request for unit tests Unit tests are needed. label Feb 14, 2024
@BoShurik
Copy link

Is it possible to use scalar types?
Also maybe use \Foo\Bar $baz instead of \Foo\Bar$baz? First one is similar to standard variable definition

@xepozz
Copy link
Member Author

xepozz commented Feb 14, 2024

Is it possible to use scalar types?

Nope, but it's good point

standard variable definition

What is it?

@BoShurik
Copy link

standard variable definition

What is it?

__construct(\Foo\Bar $baz)

space between class name and variable

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 99.85%. Comparing base (f262d5d) to head (0e2312c).

Files Patch % Lines
src/Helpers/Normalizer.php 66.66% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##              master      #87      +/-   ##
=============================================
- Coverage     100.00%   99.85%   -0.15%     
- Complexity       242      250       +8     
=============================================
  Files             16       16              
  Lines            653      672      +19     
=============================================
+ Hits             653      671      +18     
- Misses             0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xepozz
Copy link
Member Author

xepozz commented Feb 15, 2024

Added support to resolve variable-like binding.
So now it supports:

  • Single typed definition: \My\Class $param
  • Untyped definition: $fileCache
  • Union types one-by-one: \My\Class1|\My\Class2 $param -> it will look for \My\Class1 $param or \My\Class2 $param

@BoShurik
Copy link

Is \My\Class1&\My\Class2 $param supported?

@xepozz
Copy link
Member Author

xepozz commented Feb 15, 2024

Is \My\Class1&\My\Class2 $param supported?

Look at this commit. Your case is still union type. It supports discovering MyClass1 $param or MyClass2 $param.

Binding \My\Class1&\My\Class2 $cache => $cacheDefinition is not supported. Although binding any string-like keys supported, it's discovering isn't

README.md Outdated Show resolved Hide resolved
xepozz and others added 2 commits March 3, 2024 09:46
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
@samdark samdark requested review from vjik and samdark March 3, 2024 08:35
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems, this changes not need in this PR.

{
if (isset($this->definitions[$id])) {
return true;
}

if (
Copy link
Member

@vjik vjik Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that feature can degrade performance. Suggest to do it optional or need check performance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xepozz would you mind running a comparison benchmark for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:request for unit tests Unit tests are needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants