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

Lazy definition #58

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

Lazy definition #58

wants to merge 37 commits into from

Conversation

xepozz
Copy link
Contributor

@xepozz xepozz commented Nov 20, 2022

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

@codecov
Copy link

codecov bot commented Nov 20, 2022

Codecov Report

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

Project coverage is 99.55%. Comparing base (fd3bf9e) to head (e6c3e9b).
Report is 28 commits behind head on master.

❗ Current head e6c3e9b differs from pull request most recent head cf7935f. Consider uploading reports for the commit cf7935f to get more accurate results

Files Patch % Lines
src/LazyDefinition.php 78.57% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #58      +/-   ##
============================================
+ Coverage     98.43%   99.55%   +1.11%     
- Complexity      230      243      +13     
============================================
  Files            14       17       +3     
  Lines           510      667     +157     
============================================
+ Hits            502      664     +162     
+ Misses            8        3       -5     

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

@xepozz
Copy link
Contributor Author

xepozz commented Nov 20, 2022

Tests fail because of proxy-manager. It doesn't support PHP 8.1: Ocramius/ProxyManager#774

@xepozz xepozz marked this pull request as ready for review December 13, 2022 19:44
@samdark
Copy link
Member

samdark commented Jun 30, 2023

Need README update describing the feature and when it should be useful.

@samdark
Copy link
Member

samdark commented Jun 30, 2023

And a CHANGELOG entry.

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

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

Also need:

  • fix static analysis;
  • add classes from new optional dependency to composer require checker configuration for ignore;
  • add lines to changelog (new definition and change normalizier behavior)

.github/workflows/mutation.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
src/Helpers/Normalizer.php Outdated Show resolved Hide resolved
src/LazyDefinition.php Outdated Show resolved Hide resolved
src/LazyDefinition.php Outdated Show resolved Hide resolved
tests/Unit/LazyDefinitionDecoratorTest.php Outdated Show resolved Hide resolved
tests/Unit/LazyDefinitionDecoratorTest.php Outdated Show resolved Hide resolved
tests/Unit/LazyDefinitionDecoratorTest.php Outdated Show resolved Hide resolved
tests/Unit/LazyDefinitionDecoratorTest.php Outdated Show resolved Hide resolved
tests/Unit/LazyDefinitionDecoratorTest.php Outdated Show resolved Hide resolved
@vjik
Copy link
Member

vjik commented Aug 2, 2023

It remains only to add information to changelog.

src/ArrayDefinition.php Outdated Show resolved Hide resolved
xepozz and others added 4 commits August 19, 2023 12:24
# Conflicts:
#	.github/workflows/build.yml
#	.github/workflows/mutation.yml
#	tests/Unit/Helpers/DefinitionResolverTest.php
#	tests/Unit/Helpers/DefinitionValidatorTest.php
@mougrim
Copy link

mougrim commented Apr 16, 2024

Hi there!
@samdark @xepozz, do you need any help for merge this MR?

@samdark
Copy link
Member

samdark commented Apr 16, 2024

@mougrim Yeah. Testing it.

@mougrim
Copy link

mougrim commented Apr 18, 2024

@samdark I've got error:

PHP Fatal error:  Uncaught Yiisoft\Definitions\Exception\InvalidConfigException: Invalid definition: \Yiisoft\Definitions\LazyDefinition::__set_state(array(
   'definition' => 'MyClass',
   'class' => 'MyClass',
)) in vendor/yiisoft/definitions/src/Helpers/DefinitionValidator.php:56
Stack trace:
#0 vendor/yiisoft/di/src/Container.php(324): Yiisoft\Definitions\Helpers\DefinitionValidator::validate()
#1 vendor/yiisoft/di/src/Container.php(205): Yiisoft\Di\Container->validateDefinition()
#2 vendor/yiisoft/di/src/Container.php(244): Yiisoft\Di\Container->addDefinition()
#3 vendor/yiisoft/di/src/Container.php(88): Yiisoft\Di\Container->addDefinitions()

Maybe need to add some fix to \Yiisoft\Definitions\Helpers\DefinitionValidator::isValidObject, like:

return !($value instanceof DefinitionInterface) || $value instanceof ReferenceInterface || $value instanceof LazyDefinition;

@samdark
Copy link
Member

samdark commented Apr 18, 2024

@xepozz can you take a look?

@mougrim
Copy link

mougrim commented Apr 22, 2024

Quite the same trace, if I use lazy-services from yiisoft/di:

PHP Fatal error:  Uncaught Yiisoft\Definitions\Exception\InvalidConfigException: Invalid definition: \Yiisoft\Definitions\LazyDefinition::__set_state(array(
   'definition' => 'MyClass',
   'class' => 'MyClass',
)) in vendor/yiisoft/definitions/src/Helpers/DefinitionValidator.php:56
Stack trace:
#0 vendor/yiisoft/di/src/Container.php(325): Yiisoft\Definitions\Helpers\DefinitionValidator::validate()
#1 vendor/yiisoft/di/src/Container.php(209): Yiisoft\Di\Container->validateDefinition()
#2 vendor/yiisoft/di/src/Container.php(251): Yiisoft\Di\Container->addDefinition()
#3 vendor/yiisoft/di/src/Container.php(93): Yiisoft\Di\Container->addDefinitions()

I'm using the following config:

    MyClass::class => new LazyDefinition(
        MyClass::class,
        MyClass::class,
    ),

@xepozz
Copy link
Contributor Author

xepozz commented Apr 22, 2024

@mougrim Please take a look at the test if you're going to use the class directly.
yiisoft/container makes array-like definitions lazy by adding lazy => true flag into the config, e.g.:

return [
    ...
    MyClass::class => [
        'lazy'=>true,
    ],
];

@mougrim
Copy link

mougrim commented Apr 23, 2024

@xepozz, thank you!
I've tried the following config:

    MyClass::class => new LazyDefinition(
        [ArrayDefinition::CLASS_NAME => MyClass::class],
        MyClass::class,
    ),

And got the same error:

PHP Fatal error:  Uncaught Yiisoft\Definitions\Exception\InvalidConfigException: Invalid definition: \Yiisoft\Definitions\LazyDefinition::__set_state(array(
   'definition' => 
  array (
    'class' => 'MyClass',
  ),
   'class' => 'MyClass',
)) in vendor/yiisoft/definitions/src/Helpers/DefinitionValidator.php:56
Stack trace:
#0 vendor/yiisoft/di/src/Container.php(325): Yiisoft\Definitions\Helpers\DefinitionValidator::validate()
#1 vendor/yiisoft/di/src/Container.php(209): Yiisoft\Di\Container->validateDefinition()
#2 vendor/yiisoft/di/src/Container.php(251): Yiisoft\Di\Container->addDefinition()
#3 vendor/yiisoft/di/src/Container.php(93): Yiisoft\Di\Container->addDefinitions()

Maybe I do something wrong.

Array-like definition is working 👍 :

    MyClass::class => [
        'lazy' => true,
    ],

But if I forgot to install friendsofphp/proxy-manager-lts, then I got error:

PHP Fatal error:  Uncaught Yiisoft\Di\NotFoundException: No definition or class found or resolvable for "ProxyManager\Factory\LazyLoadingValueHolderFactory" while building "ProxyManager\Factory\LazyLoadingValueHolderFactory". in vendor/yiisoft/di/src/Container.php:538

Maybe better to show suggestion about installing friendsofphp/proxy-manager-lts.

@xepozz
Copy link
Contributor Author

xepozz commented Apr 25, 2024

@mougrim

I've tried the following config:

  MyClass::class => new LazyDefinition(
      [ArrayDefinition::CLASS_NAME => MyClass::class],
      MyClass::class,
  ),

It's the way if you want to use laziness directly with the yiisoft/definitions repo.
Using yiisoft/di means the only way with lazy property.

But if I forgot to install friendsofphp/proxy-manager-lts, then I got error:

Hmm, it must be shown https://github.com/yiisoft/definitions/pull/58/files#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34R42-R44

@vjik vjik added the status:under development Someone is working on a pull request. label Apr 25, 2024
@mougrim
Copy link

mougrim commented Apr 27, 2024

@xepozz

Hmm, it must be shown https://github.com/yiisoft/definitions/pull/58/files#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34R42-R44

Yes, but if you don't need lazy at once, you don't install suggested dependency.
Then when you try to use it later, you will get error:

PHP Fatal error:  Uncaught Yiisoft\Di\NotFoundException: No definition or class found or resolvable for "ProxyManager\Factory\LazyLoadingValueHolderFactory" while building "ProxyManager\Factory\LazyLoadingValueHolderFactory". in vendor/yiisoft/di/src/Container.php:538

So, you could check if validation is enabled:

if (!class_exists(LazyLoadingValueHolderFactory::class)) {
  throw new RuntimeException('For use lazy services, please install friendsofphp/proxy-manager-lts');
}

Anyway I think it's not critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants