Lack of interest re: XenForo & Add-ons 2.3.0 Release Candidate 3 Released (Unsupported)

[vbresults]

I have some opinions on the syntax of the latest RC. If I understand correctly, it went from this:

$userRepo = \XF::app()->repository('XF:User');

To this:

$userRepo = \XF::app()->repository(\XF\Repository\User::class);

To this:

$userRepo = \XF::repository(\XF\Repository\User::class);

To this:

use XF\Repository\UserRepository;

$userRepo = \XF::repository(UserRepository::class);

For me, this is the best one because it's explicit, but at the same time, it's redundant:

$userRepo = \XF::repository(\XF\Repository\User::class);

Since PHP has late static binding, this would be the most streamlined -- there is no redundancy, and it obviates renaming the classes:

$userRepo = \XF\Repository\User::instance();

That said, I think it's cool that little things like this are being given attention.

 

[Jeremy P]

The only thing which has changed are the possible arguments. The available methods are the same. \XF::repository and \XF\App::repository (among other short-hands available) all ultimately delegate to the canonical \XF\Mvc\Entity\Manager::getRepository method in both XF 2.2 and 2.3.

We just went from this in 2.2:

$userRepo = \XF::repository('XF:User');

To this in 2.3:

use XF\Repository\UserRepository;

$userRepo = \XF::repository(UserRepository::class);

However, 'XF:User' (and \XF\Repository\User::class) are still supported in 2.3 for backwards-compatibility purposes.

Since PHP has late static binding, this would be the most streamlined -- there is no redundancy, and it obviates renaming the classes:

This approach still wouldn't allow another class which uses both \XF\Entity\User and \XF\Repository\User to import them without aliasing, which is what we were after. If we were to simplify further in the future I imagine we'd probably drop support for class short-names ('XF:User') and just have a service locator method to fetch them from the container (after running them through the class extension system as necessary) via something like:

use XF\Repository\UserRepository;

$userRepo = \XF::get(UserRepository::class);

 

[vbresults]

This approach still wouldn't allow another class which uses both \XF\Entity\User and \XF\Repository\User to import them without aliasing

I think there might be a misunderstanding -- there is no usage of aliasing in this approach that imports both of them:

$userRepo = \XF\Repository\User::instance();
$user = \XF\Entity\User::instance();

• There is no redundancy in every statement
• It obviates renaming the classes in the codebase
• It's explicit
• It's as close to unopinionated as we can get (more common ::instance versus \XF::repository) -- making it easier for new developers
• The suggested method uses less code -- 44 characters vs 84 on the first usage -- the formula is (I think) 44n vs 33 + 51n

I might be misunderstanding this, but how does a service locator fit in here? Repository::instance does the same thing as \XF::repository

 

[DragonByte Tech]

The suggested method uses less code -- 44 characters vs 84 on the first usage -- the formula is (I think) 44n vs 33 + 51n

Unless you want to reference the repository multiple times per file. In such a case, the "RC3 method" wins.

 

[Jeremy P]

I think there might be a misunderstanding -- there is no usage of aliasing in this approach that imports both of them:

I mean importing the classes. The classes in your example are fully qualified. To import them without conflicting, you’d have to use aliasing:

use XF\Entity\User;
use XF\Repository\User as UserRepository;

$user = User::instance();
$userRepo = UserRepository::instance();

Adding a factory method would only affect instantiation, whereas the class names also appear in other places (like type hints) where it's much nicer to be able to import them without conflicting.

I might be misunderstanding this, but how does a service locator fit in here? Repository::instance does the same thing as \XF::repository

The redundancy we're left with now comes from our existing "service locator" methods (ie. getting an object from the container), and those methods are necessary for the class extension system to work. They are currently named after the object they create because each one would expand the same short-name into a different type of class. If we didn't have short-names, there's little reason for having separate \XF::repository and \XF::finder methods for example. One method could cover all of them, would not be redundant, and retain the benefit of being able to import them without conflicts. An ::instance factory method could do the same thing I guess, but that's orthogonal to the changes in RC 3.

 

[DragonByte Tech]

One method could cover all of them and would not be redundant, and retain the benefit of being able to import them without conflicts.

I would love to live in a world where \XF::get() was the only available method. $this->app vs $this->app() vs $this->_app is cursed and there should not be an element of RNG which one is available

Which is why I've taken to using the full \XF::app() to avoid that whole nonsense. Lets me copy code from anywhere else in my addons without fear!

 

[vbresults]

I mean importing the classes. The classes in your example are fully qualified. To import them without conflicting, you’d have to use aliasing:

To clarify, the point of suggesting fully-qualified names is that they do not need a use statement, and therefore do not introduce conflict.

The redundancy we're left with now comes from our existing "service locator" methods (ie. getting an object from the container), and those methods are necessary for the class extension system to work.

The class extension system would load through instance all the same.

If we didn't have short-names, there's little reason for having separate \XF::repository and \XF::finder methods for example.

That is what I was getting at, the same outcome with less code.

I would love to live in a world where \XF::get() was the only available method. $this->app vs $this->app() vs $this->_app is cursed and there should not be an element of RNG which one is available

Which is why I've taken to using the full \XF::app() to avoid that whole nonsense. Lets me copy code from anywhere else in my addons without fear!

$app = \XF\App::instance()

 

[DragonByte Tech]

To clarify, the point of suggesting fully-qualified names is that they do not need a use statement, and therefore do not introduce conflict.

But FQNs are longer, and I notice you ignored my previous counterpoint of needing to reference the class multiple times per file.

Even if you used a helper method like $this->getUserRepo() that’s still more code than UserRepository.

Don’t get me wrong, it is a lot of work to go through all my addons to rename things so as to not need to take advantage of BC alias hacks, but in the long run it’s worth it.

 

[vbresults]

But FQNs are longer...

I provided a concrete example for FQN length, and \XF::app() is opaque.

I notice you ignored my previous counterpoint...

No ignoring here -- when I clicked the alert, it went straight to Jeremy's comment so I didn't see your first one.

...of needing to reference the class multiple times per file.

n in the equations I provided = the number of times the class is referenced per file.

 

[DragonByte Tech]

n in the equations I provided = the number of times the class is referenced per file.

I see what you're saying now. Regardless, I'd still value a \XF::get() over public static function instance() (or public static function get()) because it means we don't need to duplicate the instantiator into multiple different AbstractX classes and/or add an interface for this.

Also, it would allow for easier context injection, better control over caching, etc.

Lastly, for built-in XF classes it's not that big of a difference, but consider this:

\DBTech\eCommerce\Repository\Product::instance();

use DBTech\eCommerce\Repository\ProductRepository;
\XF::get(ProductRepository::class);

49n vs 50 + 35n.

Going with a real-world example, the most usages that repo gets in one file is 5. So if we do the maths; with your method that's 245 vs 225.

I also have add-ons with even longer names, such as UserUpgradeCoupon with a repository named Coupon, and 3 instances of the repo in one file would see savings using the proposed \XF::get(). I didn't do the maths on the minimum number of instances of the above Product repo. You get the point.

---

I also think you might be missing part of Jeremy's point.

To clarify, the point of suggesting fully-qualified names is that they do not need a use statement, and therefore do not introduce conflict.

The point isn't whether or not use statements are needed, but whether they are desired. Personally, I like short names in code but prefer FQNs in docblocks. If the classes themselves are self descriptive (e.g. UserController/UserRepository/User) it means the code (with use statements) looks a lot more concise and clean, at least in my opinion.

When reading the code normally, we don't need to know the full path to the class. For example, \DBTech\eCommerce\Service\Product\Create. CreateService is perfectly fine when reading code in the context of the Product controller.

Not applicable to XF (yet ) but the XF 2.3 versions of my add-ons will require PHP 8.0. That means union types are available to me. This is an extreme example that isn't a real-world example, but for illustrative purposes, you tell me what's more readable:

protected function getProductAddEditService(): \DBTech\eCommerce\Service\Product\Create|\DBTech\eCommerce\Service\Product\Edit
    
protected function getProductAddEditService(): CreateService|EditService

I am of course aware that aliases would be available for these kind of examples, but at least in phpStorm, if I Cmd-Click on an aliased class, it first takes me to the alias definition in the use statement, and I'd then have to additionally Cmd-Click the "real" class to actually open it. With a non-aliased use, it takes me directly to the class in the code itself.

All of that being said, I do understand it's a matter of preference. I think I've made a good case for why this new way of doing it has some clear and present benefits

 

[vbresults]

I see what you're saying now. Regardless, I'd still value a \XF::get() over public static function instance() (or public static function get()) because it means we don't need to duplicate the instantiator into multiple different AbstractX classes and/or add an interface for this.

Also, it would allow for easier context injection, better control over caching, etc.

There is no explicit need to duplicate the definition of instance or add an interface because we can use traits.

Lastly, for built-in XF classes it's not that big of a difference, but consider this:

use DBTech\eCommerce\Repository\ProductRepository;
\XF::get(ProductRepository::class);

49n vs 50 + 35n.

Going with a real-world example, the most usages that repo gets in one file is 5. So if we do the maths; with your method that's 245 vs 225.

In that case, the suggested method would be shorter because we can alias off of a shorter base name.

use \DBTech\eCommerce\Repository\Product as ProductRepo;
ProductRepo::instance();

50 + 35n vs 56 + 24n

I also have add-ons with even longer names, such as UserUpgradeCoupon with a repository named Coupon, and 3 instances of the repo in one file would see savings using the proposed \XF::get(). I didn't do the maths on the minimum number of instances of the above Product repo. You get the point.

I also think you might be missing part of Jeremy's point.

The point isn't whether or not use statements are needed, but whether they are desired. Personally, I like short names in code but prefer FQNs in docblocks. If the classes themselves are self descriptive (e.g. UserController/UserRepository/User) it means the code (with use statements) looks a lot more concise and clean, at least in my opinion.

When reading the code normally, we don't need to know the full path to the class. For example, \DBTech\eCommerce\Service\Product\Create. CreateService is perfectly fine when reading code in the context of the Product controller.

Not applicable to XF (yet ) but the XF 2.3 versions of my add-ons will require PHP 8.0. That means union types are available to me. This is an extreme example that isn't a real-world example, but for illustrative purposes, you tell me what's more readable:

protected function getProductAddEditService(): \DBTech\eCommerce\Service\Product\Create|\DBTech\eCommerce\Service\Product\Edit
 
protected function getProductAddEditService(): CreateService|EditService

A:

use \DBTech\eCommerce\Service\Product\{Create as CreateProductService, Edit as EditProductService};

protected function getProductAddEditService(): CreateProductService | EditProductService {
    return CreateProductService::instance();
}

B:

use \DBTech\eCommerce\Service\Product\{Create as CreateProductService, Edit as EditProductService};

protected function getProductAddEditService(): CreateProductService | EditProductService {
    return \XF::get(CreateProductService::class);
}

One of these is unopinionated, less redundant and slightly shorter.

I am of course aware that aliases would be available for these kind of examples, but at least in phpStorm, if I Cmd-Click on an aliased class, it first takes me to the alias definition in the use statement, and I'd then have to additionally Cmd-Click the "real" class to actually open it. With a non-aliased use, it takes me directly to the class in the code itself.

In PHPStorm, a separate shortcut to go to straight to the class definition exists.

 

[Jeremy P]

I understand it can be a matter of preference, but using a service locator to fetch an object from the container shouldn't be too unfamiliar to developers. It's a common concept in modern frameworks. Dependency injection is usually preferable of course, but wiring up the container manually is a bit cumbersome.

However ultimately the point here is that making unqualified class names unique and importing them with use confers additional benefits beyond instantiation. The experience in modern IDEs and editors is quite nice too, as you can just type eg. UserRep and get the class autocompleted, inserted, and imported automatically. Imports also give you a place where you can see all the classes a given class interacts with in one place. We could indeed simplify instantiation further, but that's a separate concern for another day.

 

[vbresults]

I understand it can be a matter of preference, but using a service locator to fetch an object from the container shouldn't be too unfamiliar to developers. It's a common concept in modern frameworks. Dependency injection is usually preferable of course, but wiring up the container manually is a bit cumbersome.

However ultimately the point here is that making unqualified class names unique and importing them with use confers additional benefits beyond instantiation. The experience in modern IDEs and editors is quite nice too, as you can just type eg. UserRep and get the class autocompleted, inserted, and imported automatically. Imports also give you a place where you can see all the classes a given class interacts with in one place.

You've made solid points. 2.3's new naming convention is the right way forward.

We could indeed simplify instantiation further, but that's a separate concern for another day.

This part of the suggestion still stands; IMO 2.3 would be the better release to implement it in if you intend to go that way since you are already doing the class renaming.

 

[Jeremy P]

We already have some loose plans there (which I've hinted at), but they're likely to coincide with other changes in a future release because there are other moving parts involved.

 

[stromb0li]

However, 'XF:User' (and \XF\Repository\User::class) are still supported in 2.3 for backwards-compatibility purposes.

Semi-related. If using the legacy way in 2.3, does that trigger a warning anywhere? In other words, is there a way to strictly enforce "2.3 only" way, so that it forces me as a developer to adopt the better practice / catch overlooked sections I may have missed?

 

[Jeremy P]

It's not formally deprecated yet, but perhaps in a future version we'll emit deprecation warnings for them in debug/development mode. In the meantime doing a project wide search for '[\\A-Z][\\\w]*:[\\A-Z][\\\w]*' should yield most if not all of them assuming your vendor/add-on IDs are capitalized.