Skip to content

Conversation

@xepozz
Copy link
Member

@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
Member 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.

@vjik vjik force-pushed the lazy-definition branch from 7f2f728 to e03e225 Compare July 1, 2023 13:54
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)

xepozz and others added 3 commits August 1, 2023 22:16
@vjik
Copy link
Member

vjik commented Aug 2, 2023

It remains only to add information to changelog.

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
Member 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
Member 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.

6 participants