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

Add legacy autoloader to support Matomo namespaces in 3.X #16069

Merged
merged 5 commits into from Jun 17, 2020

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Jun 15, 2020

Refs #12420 (comment)

The purpose is to make updates to a future Matomo version smoother where we maybe use Matomo namespace instead of Piwik

Tested it eg in 3.x-dev by adding a file core/Foo.php with content

<?php
namespace Matomo;

class Foo {
    public function get()
    {
        return '42';
    }
}

then in core/dispatch.php tested it using

$x = new \Piwik\Foo();
echo $x->get();

and using

$x = new \Matomo\Foo();
echo $x->get();

In all cases the class was resolved correctly with no issues.

@tsteur tsteur added the Needs Review PRs that need a code review label Jun 15, 2020
@tsteur tsteur added this to the 3.13.7 milestone Jun 15, 2020
{
if (strpos($className, 'Matomo\\') === 0) {
$newName = 'Piwik' . substr($className, 6);
if (class_exists($newName) && !class_exists($className, false)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

btw the second class_exists is needed because things could happen like

* new \Matomo\Foo
* It then does `class_exists(\Piwik\Foo)`
* It then loads `core/Foo.php`
* This class defines `Matomo\Foo`
* Then it adds alias `Piwik\Foo` to `Matomo\Foo`
* This first `class_exists($newName)` basically caused the file `core/Foo.php` to load which defines `Matomo\Foo` so we no longer want to execute `class_alias(\Matomo\Foo, Piwik\Foo)` since the first class_exists actually defined the class already indirectly.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Looks good. We just need to keep in mind, that if we merge this to 3.x-dev, the changes to LegacyLoader would get merged to 4.x-dev later. So if we don't want to have the changes in 4.x-dev, we need to undo them in the next merge to 4.x-dev

@tsteur
Copy link
Member Author

tsteur commented Jun 17, 2020

👍

@tsteur tsteur merged commit d25356a into 3.x-dev Jun 17, 2020
@tsteur tsteur deleted the legacyautoloader branch June 17, 2020 20:52
jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants