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

Still a small issue on ObjectRegistry load aliasing #17620

Open
dereuromark opened this issue Mar 11, 2024 · 6 comments
Open

Still a small issue on ObjectRegistry load aliasing #17620

dereuromark opened this issue Mar 11, 2024 · 6 comments
Labels
Milestone

Comments

@dereuromark
Copy link
Member

Description

https://github.com/dereuromark/cakephp-expose/actions/runs/8236778340
shows that there is a regression if you load behaviors using

$this->getController()->$modelName->addBehavior('Expose.Superimpose', $this->getConfig());

where the config does not contain a className key even

Apparently, the behavior gets added with alias 'Expose.Superimpose' instead of 'Superimpose'
and thus the code later trying to remove it fails

$postsTable->removeBehavior('Superimpose');

I guess we need to check for the alias containing a dot and

  • throw exception if className exists but does not match
  • rename to alias without plugin prefix if both are equal or if className is not present

CakePHP Version

5.0.6

PHP Version

8.2

@dereuromark dereuromark added this to the 5.0.7 milestone Mar 11, 2024
@dereuromark
Copy link
Member Author

dereuromark commented Mar 11, 2024

The main issue I encounted while trying to fix this is that there are (IMO bad) existing behaviors and tests in place that fail here..

/**
     * test loading helpers with dotted aliases
     */
    public function testLoadPluginHelperDottedAlias(): void
    {
        $this->loadPlugins(['TestPlugin']);

        $result = $this->Helpers->load('thing.helper', [
            'className' => 'TestPlugin.OtherHelper',
        ]);
        $this->assertInstanceOf(OtherHelperHelper::class, $result, 'Helper class is wrong.');
        $this->assertInstanceOf(
            OtherHelperHelper::class,
            $this->Helpers->get('thing.helper'),
            'Class is wrong'
        );

Why would we ever allow a dotted alias? That seems to break thing for no good reason.

I fixed up the plugin for now ( dereuromark/cakephp-expose@738e89b ).

If we are deadlocked for 5.0, I recommend we clean this up for 5.1?

@ADmad
Copy link
Member

ADmad commented Mar 11, 2024

That example is indeed a bit of a doozy. I wouldn't mind getting rid of that in 5.1. So if className is specified then don't allow a dot in the alias.

@dereuromark
Copy link
Member Author

dereuromark commented Mar 11, 2024

I prepared a branch to see the issues we currently have for this:
#17623
Quite a bit of Cache object registry is involved here.

For an easier upgrade path we could also auto-switch existing dots to - for example.
In many cases it is more to generate the registry elements, and not about the specific alias.

@markstory
Copy link
Member

Why would we ever allow a dotted alias? That seems to break thing for no good reason.

Because it enables usage like helpers->load('Plugin.CustomHelper')

So if className is specified then don't allow a dot in the alias.

This sounds like a good compromise. We can retain the short alias/classname specifier, and when you use options to define the className you can't use a dotted alias.

@dereuromark
Copy link
Member Author

Because it enables usage like helpers->load('Plugin.CustomHelper')

Not exactly, as those are already normalized, even if config array is passed - as long as className is not set.

The issue here is with things that are using both at the same time, see my last comment and the linked CI run.

@markstory markstory modified the milestones: 5.0.7, 5.0.8 Apr 6, 2024
@dereuromark
Copy link
Member Author

dereuromark commented Apr 6, 2024

I just copy the result of the stricter test run here for easier review of the issues remaining:

There were 34 errors:

1) Cake\Test\TestCase\Cache\CacheTest::testConfigDottedAlias
InvalidArgumentException: Dot syntax is not supported for `className` registry usage.

/home/runner/work/cakephp/cakephp/src/Core/ObjectRegistry.php:96
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:153
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:216
/home/runner/work/cakephp/cakephp/tests/TestCase/Cache/CacheTest.php:471
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:1122
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:654
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestRunner.php:105
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:488
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:62
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/Application.php:199

2) Cake\Test\TestCase\Cache\CacheTest::testGroupConfigs
InvalidArgumentException: Dot syntax is not supported for `className` registry usage.

/home/runner/work/cakephp/cakephp/src/Core/ObjectRegistry.php:96
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:153
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:216
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:494
/home/runner/work/cakephp/cakephp/tests/TestCase/Cache/CacheTest.php:490
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:1122
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:654
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestRunner.php:105
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:488
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:62

--

There were 2 failures:

1) Cake\Test\TestCase\Cache\CacheTest::testGroupConfigsThrowsException
Failed asserting that exception of type "InvalidArgumentException" matches expected exception "Cake\Cache\Exception\InvalidArgumentException". Message was: "Dot syntax is not supported for `className` registry usage." at
/home/runner/work/cakephp/cakephp/src/Core/ObjectRegistry.php:96
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:153
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:216
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:494
/home/runner/work/cakephp/cakephp/tests/TestCase/Cache/CacheTest.php:558
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:1122
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:654
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestRunner.php:105
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:488
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:62
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/Application.php:199
.

/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:106
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:51
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/Assert.php:[187](https://github.com/cakephp/cakephp/actions/runs/8240113324/job/22534833843#step:16:188)0
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:2147
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:1128
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:654
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestRunner.php:105
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:488
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:62
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/Application.php:199

2) Cake\Test\TestCase\Cache\CacheTest::testGroupConfigsThrowsOldException
Failed asserting that exception of type "InvalidArgumentException" matches expected exception "Cake\Cache\Exception\InvalidArgumentException". Message was: "Dot syntax is not supported for `className` registry usage." at
/home/runner/work/cakephp/cakephp/src/Core/ObjectRegistry.php:96
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:153
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:216
/home/runner/work/cakephp/cakephp/src/Cache/Cache.php:494
/home/runner/work/cakephp/cakephp/tests/TestCase/Cache/CacheTest.php:567
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:1122
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:654
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestRunner.php:105
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:488
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:62
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/Application.php:[199](https://github.com/cakephp/cakephp/actions/runs/8240113324/job/22534833843#step:16:200)
.

/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:106
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:51
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/Assert.php:1870
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:2147
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:1128
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:654
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestRunner.php:105
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestCase.php:488
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/Framework/TestSuite.php:341
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:62
/home/runner/work/cakephp/cakephp/vendor/phpunit/phpunit/src/TextUI/Application.php:199

@markstory markstory modified the milestones: 5.0.8, 5.0.9 May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants