Replies: 1 comment
-
I will also mention this, since its relevant and I omitted in my earlier post. If you implement psr 11 and your AddonManager becomes a subclass of the implementer then it will control the creation context of all Addons. Usually by ensuring they are of a given type. Say for example it checks to make sure that they all implement the AddonInterface. Which AbstractAddon would. Your Addon run() method could be moved could have access to that subclass. Your AddonManager config could alias callable service names to registered Addons. Essentially making them callable via the AbstractAddOn __call method, which would just proxy to the run() method. This would remove the need for glob'ing the target path and save the overhead of processing each file and sorting out how to call the addon blah blah. If the Addon is registered with the container and its invokable, then it gets called, otherwise an instance is returned by the AddonManager. |
Beta Was this translation helpful? Give feedback.
-
This is just some general thoughts (it's not a critique, just some questions/thoughts that come to mind while digging into the code).
Light-Portal/src/Sources/LightPortal/AddonHandler.php
Line 46 in fa631d9
In the above referenced class its modeled as a singleton. Before I used singletons I would just provide a factory for the AddonHandler class. Internally the factory can track the instance it has created, if it has already been created, then return that instance, otherwise return a new instance. This would allow a very simple refactor when/if SMF moves to psr 11. More importantly it would remove your need for a singleton. 95% of the time if a singleton is needed its a symptom of an underlying design flaw. Not the design flaw is yours, by any means. In your case its a reflection of the current state of the SMF codebase, this statement applies as much if not more to 3.0 currently as it does to the current stable release. The need for SMF to be built with singletons is not a design choice it's a symptom of a flawed service layer, or more correctly the total lack thereof.
(related to above)
Another point I would make is that AddonHandler more precisely reads as a AddonManager (sorta... honestly the object is doing to many things. The service needs split) because if the forum moves psr 11 then this class should really become a subclass of the forums psr 11 implementer. I realize the naming is just semantic. Just keep in mind that the naming has wider implications for those coming from different types of applications.
If it becomes a subclass of a psr 11 container it would allow Addon to be an abstract class "Addons" extend. If your addon concrete implementations implement an __invoke method then they are all callables. It simplifies your calling code since you have gained first class callable syntax by moving to 8.1.
loadAssets() (I picked this one randomly since I can see how it could benefit the mod as whole by being psr 14 driven).
Not sure why you are not exposing this as an event so that addons/plugins can attach listeners to allow them to load their needed assets. Maybe this is done in another way and I just missed it. I will mention there are few approaches better suited for decoupling code than psr 14.
Honestly, I think it would be beneficial for you to implement psr 11. If you do then it solves the first point I made above.
Bugo\LightPortal\Helper
This helper needs split. A trait should never compose methods that are not valid for the current objects state. It also creates a lot of complexity in your Unit test. Another reason I "generally" pair traits to the interface contract that they compose. Unit test are really contract test. Only methods valid for the contract belong. They are very convenient but they can also be easily abused.
Remember.
Dry OCP (singletons kill OCP)
I'm gonna leave it here for now.
Beta Was this translation helpful? Give feedback.
All reactions