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

Use rebranded composer components #15253

Merged
merged 8 commits into from Jan 27, 2020
Merged

Use rebranded composer components #15253

merged 8 commits into from Jan 27, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Dec 9, 2019

refs #12519

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Dec 9, 2019
@sgiehl sgiehl added this to the 4.0.0 milestone Dec 9, 2019
@sgiehl sgiehl force-pushed the userebrandedcomponents branch 2 times, most recently from 309d343 to 84937a5 Compare December 19, 2019 09:23
@sgiehl
Copy link
Member Author

sgiehl commented Dec 30, 2019

@diosmosis @tsteur @mattab Switching to the rebranded packages (with new namespaces) might maybe cause problems while updating, as some plugins might still use the old namespace until they are updated.
Any ideas how we should solve that issue? Should we update the plugins before, so they use the actual available namespace? Or shall we still include both packages in Matomo 4 (old and rebranded ones), so both namespaces are still available - and remove the old one in Matomo 4.1 or so?

@tsteur
Copy link
Member

tsteur commented Dec 30, 2019

and remove the old one in Matomo 4.1 or so?

I reckon this won't be possible since some people will update from older 3.x or 2.X to 4.1.

Updating plugins first can cause issues too if anything fails there.

Ideally the components would have both the new Matomo and the old Piwik classes but that might not be easily doable. Although the components typically maybe don't have too many classes so could be done maybe. Otherwise I suppose it should be fine maybe as long as the plugins don't access any component during update? They might though... also people manually updating core etc will have problems.

Do we even need to update the components from Piwik to Matomo or wait until we do this also for core?

@diosmosis
Copy link
Member

Ideally the components would have both the new Matomo and the old Piwik classes but that might not be easily doable. Although the components typically maybe don't have too many classes so could be done maybe.

Could this be done via inheritance? Ie:

namespace Piwik;

class Abc extends Matomo\Abc {
  // empty
}

@sgiehl
Copy link
Member Author

sgiehl commented Dec 30, 2019

Could this be done via inheritance? Ie:

I guess that should work. Should we add those classes to the packages or to Matomo itself?

@tsteur
Copy link
Member

tsteur commented Dec 30, 2019

Not sure what's easier?

@tsteur
Copy link
Member

tsteur commented Dec 30, 2019

I suppose we would need to make sure the autoloader still finds the files etc.

@tsteur
Copy link
Member

tsteur commented Dec 30, 2019

Very crazy thought... can the auto loader do that? Recognise Piwik namespace, then look for the matomo file that includes Matomo class, then do dynamically a if (!class_exists($piwikNamespaceClass, $doNotLoadFile)) { class $piwikNamespaceClass extends $matomoNamespaceClass }

Not sure if we can extend classes that way dynamically...

@sgiehl
Copy link
Member Author

sgiehl commented Dec 30, 2019

@tsteur I'll give it a try and implement it that way if possible

@sgiehl
Copy link
Member Author

sgiehl commented Dec 31, 2019

@tsteur found an even simpler solution using class_alias:

public function load_class($className)
{
if (strpos($className, 'Piwik\\') === 0) {
$newName = 'Matomo' . substr($className, 5);
if (class_exists($newName)) {
class_alias($newName, $className);
}
}
}

@tsteur
Copy link
Member

tsteur commented Dec 31, 2019

That's pretty cool 👍

@sgiehl
Copy link
Member Author

sgiehl commented Jan 15, 2020

Should be ready for review. Remaining package is piwik/device-detector. But I will do some more refactorings there before releasing a new rebranded version.

@sgiehl sgiehl removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jan 15, 2020
@sgiehl sgiehl added the Needs Review PRs that need a code review label Jan 15, 2020
}
}

LegacyAutoloader::register();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I missed it but could we add maybe a few basic tests that this works?

In general behaviour seems otherwise good: https://3v4l.org/TAS8d

Can we assume, that if a file containing a Matomo class was loaded manually through include/require, then the logic would not work? (Probably not an issue but something to be aware of eg if we wanted to do that for all classes in core).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that works as well. Added some simple tests for it. See de2fea6

@tsteur tsteur merged commit 907a3d6 into 4.x-dev Jan 27, 2020
@tsteur tsteur deleted the userebrandedcomponents branch January 27, 2020 21:20
@mattab mattab changed the title Use rebranded components Use rebranded composer components Sep 29, 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

3 participants