Subclasses for aliased classes

[JN-Jones]

As @wpillar suggested here: #99 (comment) we should create sub classes for classes we usually alias (like Manager or Repository)

• DaveJamesMiller\Breadcrumbs\Manager should be subclassed to Breadcrumbs and stored in a sensible namespace.

[JN-Jones]

Another (probably better) way would be to use class_alias. A new file somewhere which is included as soon as possible (probably in the bootstrap file or the service provider) which either calls class_alias for each class or (easier to maintain) contains a basic array which then is passed in a foreach to class_alias.

File would be something like this then:

<?php

$aliasedClasses = [
    'DaveJamesMiller\Breadcrumbs\Manager' => 'DaveJamesMiller\Breadcrumbs\Breadcrumbs',
    // More aliases go here
];

foreach ($aliasedClasses as $original => $alias) {
    class_alias($original, $alias);
}

We could also put the array in a (new) config file and only keep the foreach in that file.

[wpillar]

I think this is the only class that needs to be subclassed and that's because we don't own it ourselves. Personally I think subclassing is a lot less work and overhead than setting up class aliases in the way you describe.

[JN-Jones]

It's a one liner if we add it to the service provider.

Also laravel has a lot of classes named Repository (season, config, ...) or Manager which are already aliased in some places and should be aliased on others too.

[wpillar]

I still don't think class_alias() is the best approach. It obscures things.

How is my IDE or editor going to know about the alias? If I see a Breadcrumbs dependency on a class and then search for it and don't find it, that's going to confuse me and many others, needlessly so. If we simply subclass some of them, IDEs will still be able to click through to classes, and developers can easily search for classes and just see that they are empty subclasses.

My vote is -1 for class_alias().

[euantorano]

I agree, I dislike using alias as it makes things much less understandable at first glance.

On 21 May 2015, at 18:22, Will Pillar notifications@github.com wrote:

I still don't think class_alias() is the best approach. It obscures things.

How is my IDE or editor going to know about the alias? If I see a Breadcrumbs dependency on a class and then search for it and don't find it, that's going to confuse me and many others, needlessly so. If we simply subclass some of them, IDEs will still be able to click through to classes, and developers can easily search for classes and just see that they are empty subclasses.

My vote is -1 for class_alias().

—
Reply to this email directly or view it on GitHub.

[JN-Jones]

So we're going with subclasses here? Any preferred way where they should be saved and in which namespace?

[euantorano]

Yeah, subclasses would be my preference.

On 30 Oct 2016, at 07:54, Jones notifications@github.com wrote:

So we're going with subclasses here? Any preferred way where they should be saved and in which namespace?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[JN-Jones]

Something else I've noticed: There's a facade Breadcrumbs which we use eg in ConversationController. IIRC we decided at some point to avoid using facades so these occurences should be updated too shouldn't they?

[euantorano]

Yeah, they should be.

On 31 Oct 2016, at 08:38, Jones notifications@github.com wrote:

Something else I've noticed: There's a facade Breadcrumbs which we use eg in ConversationController. IIRC we decided at some point to avoid using facades so these occurences should be updated too shouldn't they?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[JN-Jones]

Ok, apparently I've been inactive for way too long. The Facade is used in the base view to call renderIfExists so creating a subclass named Breadcrumbs doesn't seem to work. And even though I created a Twig Extension to avoid naming conflicts it seems not to use the package service provider anymore which results in missing breadcrumbs and views. I've tried to play with our service provider but wasn't able to bind the classes as they should be. Probably best if you could take a look.

[euantorano]

Service providers are loaded in the order they're defined in the config, so double check the breadcrumbs service provider is above the twig one.

I'll have a look over the weekend, we can talk through it and possible approaches then.