? Pending

User tests: Successful: Unsuccessful:

avatar nikosdion
nikosdion
5 Jan 2022

Proof of concept / rough draft of the incidental points discussed with @HLeithner on #36042

I will also tag @wilsonge because he's interested in improving MVC and events handling could be very important towards that goal. If you guys think of anyone else who should review it, tag 'em.

Summary of Changes

I made a few additions in the CMS Events package.

Results in events

ResultAwareInterface and its implementing Traits aim to provide a simpler way for Events and event handlers to return values through the events dispatcher.

Right now we only have legacy listeners returning results. The CMSPlugin::registerLegacyListener implements this internally by appending the result of the legacy plugin event handler (plugin method) to a named array event attribute called result.

Moving legacy listeners to event handlers this becomes clunky as I need to do things like this:

$result = $event->getArgument('result') ?? [];
$event->setArgument('result', array_merge($result, [$thisHandlerResult]);

It's very easy to screw up and overwrite the result.

Using the new interface and the traits you can simply do:

$event->addResult($thisHandlerResult);

It's neater, far easier to read (especially for array results) and doesn't let the developer mess up the results array. There's a provision to make setArgument('result', ....) no longer work FOR NEW EVENTS ONLY (event names introduced in 4.1 or later versions) and ONLY if these are NEVER going to be used through a legacy handler, since the CMSPlugin will always try to use setArgument for the results of legacy listeners. In 5.0 the only option will be disabling setArgument for the result named argument since we won't need to support legacy handlers in CMSPlugin anymore.

There is also a provision to make it work with immutable events too, appending results being the only mutable part of the event. Purists will cringe BUT being practical is more important. In most cases we want the arguments we pass to the event to be fully immutable and yet we need to receive a result. That's what I was telling @HermanPeeren and @nibra (back in 2016 in the very first J4WG) was my only trepidation on the proposed orthogonality of events handling. We definitely have tons of use cases where returning a result is necessary.

Results are type–checked before being appended to the named argument 'result'. I have included a number of traits which implement the type checks to avoid boilerplate code extravaganza. The traits also support nullable types and Joomla's stupid legacy scalar-or-boolean-false union type (e.g. array|false). By default BOTH nullable AND false-able are disabled and have to be explicitly enabled in the constructor of the event if an event uses them. I strongly recommend against false-able types. They are an icky leftover from the Mambo days, before nullable types were a thing and before Joomla started using Exceptions for communicating errors (still an ongoing process as Harald can attest).

Auto-mapping core event names to concrete event classes

See the CoreEventAware trait. You give it an event name and it tells you which concrete event class you should use. I am using this in the application's EventAwareTrait to migrate triggerEvent from generic events to concrete events where possible. This will allow us to rewrite core plugins to use concrete classes EVEN THOUGH third party developers may still be using the application's tiggerEvent() method to call them. This will be extraordinarily important when we create a concrete class for onContentPrepare ?

Reshaping argument for b/c

It's best to read the comment block in ReshapeArgumentsAware trait.

The short version is that concrete events use named arguments whereas legacy event listeners use positional arguments. For the two to mix well in the transition period of Joomla 4.x, before we move to concrete event classes and named arguments only, we need to ensure that all named arguments are also given in the correct number and order to work as positional arguments by CMSPlugin::registerLegacyListener.

That trait handles that, it just needs to be called from the constructor of the concrete event. You give it the array of named arguments passed, the expected argument names and optionally an array with the default values for optional parameters (think about the optional arguments in legacy plugin listener methods).

!!! THIS IS A DRAFT PULL REQUEST !!!

I am just submitting a code dump that came out of my head. I have not tested it at all. It definitely needs unit test, it probably needs some finessing.

The purpose of this draft PR is to show you what I had in mind about Events back in 2016 plus everything I have hitherto learned and thought of. Somewhere between getting married, moving houses (thrice), having a child, the need to rewrite my software for Joomla 4 and PHP 8, and a global pandemic I never managed to get there. Well, maybe it's a good thing because in those 6 ½ years I got experience (and PHP moved on in leaps and bounds), allowing me to write much more economical code than I had originally envisioned.

I want your feedback on the direction I am taking this. Does this look good or I am pulling this to the wrong direction? Questions? Ideas? Profound tidbits of knowledge? Everything's welcome and nothing's set in stone.

avatar nikosdion nikosdion - open - 5 Jan 2022
avatar nikosdion nikosdion - change - 5 Jan 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 5 Jan 2022
Category Libraries
avatar HLeithner
HLeithner - comment - 6 Jan 2022

I took only a quick look and will come back to this next week, because I'm busy this weekend.

But some trivial questions.

  1. eventNameToConcreteClass needs to old all legacy events incl. the mapping to the new event?
  2. the code in getEventClassByEventName for onWebAssetRegistryChangedAsset is just example code if some special is needed that couldn't solved by the array right?
  3. ReshapeArgumentsAware will be removed in j5 right?
  4. ResultAware (and interface) lives longer? So would you expect we keep "result" as default? variable name? I tought we would have more meaningful names for each event. ex. GetIconEvent would set "icon" and not "result" or I'm wrong?
  5. looking at ResultTypeArrayAware, would mean we keep the "broken" results for refactored events? or can be proved correct types for alternative values getArgument('eventSpecificNotFalse') and at the same time have getArguemnt('result') is false instead of null?
  6. Also the now introduced traits (ReshapeArguments) should be marked as internal so 3rd party developer shouldn't use them to convert there events.

Like sad just a quick look because busy...

avatar nikosdion
nikosdion - comment - 6 Jan 2022
  1. Correct. It needs to have all the event names with concrete event classes. Once we are done migrating all events it will indeed contain all of the core event names.
  2. Correct. So far it was the only event name which could not be resolved with the array. This could also be the case if we have for example controllers create onComSomethingControllernameBeforeTaskname events (random example, not necessarily something we would be doing).
  3. Correct. This is a b/c shim needed only for the transitional period (Joomla 4) where we still have CMSPlugin create legacy event listeners.
  4. Correct. We should always use a named attribute called result for output purposes. This is essentially a reserved name. Any developer working with any unfamiliar event would know that EITHER the event implements ResultAwareInterface and uses addResult() to add to the result named argument OR it does not support return values. Having it uniform across events minimises the chance of screw-ups of colossal proportions. Ask me how I know :)
  5. I assume by broken you mean the "false-able" non-Boolean types? These are currently opt-in and that would be a feature I'd like remove in Joomla 5. Results should be a. scalars; b. union types consisting only of class names; or c. nullables. Union types with non-Boolean values and Boolean false are icky and need to be removed by Joomla 5. They were used instead of Exceptions which we both agree is not good at all. So, this part of the ResultAware traits would indeed be removed in Joomla 5, that's why it's marked deprecated.
  6. I disagree. We should mark them deprecated. During the transitional period 3PDs will also need to use these traits. Remember that 3PDs like me define their own events. When I move my code into concrete event classes I will need to use ReshapeArgumentsAware to allow other people's code handling my software's events to continue working.

Well, you said you took a quick look but you really caught every single major thing that went through my mind as I was writing this code :)

avatar chmst chmst - change - 31 Jan 2022
Labels Added: ?
avatar nikosdion
nikosdion - comment - 26 Feb 2022

@HLeithner It's been nearly two months. Can you please provide some feedback so I know if this draft should be converted to a real PR? You may want to consult with @nibra @bembelimen and @roland-d since this touches all Joomla versions from 4.1 onwards.

avatar nikosdion nikosdion - change - 4 Mar 2022
Labels Added: ?
Removed: ?
avatar nikosdion
nikosdion - comment - 4 Mar 2022

@HLeithner Regarding setArgument: you are thinking that all events are immutable which is not the case for two reasons.

Some Joomla events will have to remain mutable. Think about onContentPrepare, for example. It's meant to be a chain. Each event handler modifies the text which is passed as an input argument. The next event handler works on the modified text and so on and so forth.

Third party developers have legitimate uses cases where they want mutable events. If the argument to be directly modified is not an object we need a mutable event and setArgument.

Finally, keep in mind that immutable events already forbid the use of setArgument.

Therefore it's up to us to slowly migrate all core Joomla events with immutable arguments to be immutable events. That's the correct architectural solution and what we had agreed back in August 2015 in Denmark. @nibra and @HermanPeeren had argued that all events should be immutable but I am saying that this isn't possible without a hard b/c break. For example, onContentPrepare would have to always accept an object instead of plain text but that would break b/c as far as I can see. Maybe we COULD throw a deprecated warning in the log throughout Joomla 4.x when this happens and move these events to only accept objects as arguments and have them extend the immutable event, thereby effectively disabling setArgument without screwing 3PDs who are in need of a mutable event.

PS: I am currently undercaffeinated and sleep deprived. If I said something stupid feel free to correct me.

avatar HLeithner
HLeithner - comment - 4 Mar 2022

It's not that I expect that all events are/should be immutable, I just wondering if all core events should follow this pattern. Especially as there is a concept about splitting events and hooks which I would expect to be over engineering this part a bit.

Of course we should be as b/c as possible, also if we can't move all events to immutable. I would expect the onContentPrepare event is the most used in Joomla, kill/reworking this without a b/c layer for longer then 4 (J 5-6 or forever) seems not really user and 3rd party friendly. ymmv.

I think the rest looks good and we should aim to get this into 4.2 so 3rd party devs have enough time for j5 to move forward.

btw. there is an initiative to create Joomla developer "only" documentation written in markdown using a github repo where we can/should/would document the event system and the migration for developer. So if you write some documentation here in markdown we/you could easily move it to the new repo as soon as they are ready to accept PRs.

avatar roland-d
roland-d - comment - 4 Mar 2022

@nikosdion I have looked through the code, read all the comments and I am happy to see work is being done on the the event system. I have not added any feedback on the code as it is still draft. I like where this is going but haven't tested anything yet.

If we can get this into a production state I would be glad to include it in 4.2.

avatar joomla-cms-bot joomla-cms-bot - change - 4 Mar 2022
Category Libraries Unit Tests Repository Administration com_associations com_banners com_cache com_categories com_contact com_content com_fields com_finder com_installer com_languages JavaScript com_media NPM Change
avatar nikosdion nikosdion - change - 4 Mar 2022
The description was changed
avatar nikosdion nikosdion - edited - 4 Mar 2022
avatar joomla-cms-bot joomla-cms-bot - change - 4 Mar 2022
Category Unit Tests Repository Administration com_associations com_banners com_cache com_categories com_contact com_content com_fields com_finder com_installer com_languages JavaScript com_media NPM Change Libraries
avatar nikosdion
nikosdion - comment - 4 Mar 2022

@roland-d I have rebased this upon 4.2-dev and removed the draft status. Feel free to do a regular code review.

If the concept works for you and it gets merged I can start working on the next logical steps: addressing the places where Joomla core events are defined by name and adding concrete classes to each core event not already having a concrete class.

Let me explain the first point, as it'd logically be the next PR.

BaseDatabaseModel, ListModel, and AdminModel let the developer define the names of the events which will be called under some circumstances, e.g. AdminModel lets you set the event_after_delete property to the name of the event to be used after deleting a record, by default onContentAfterDelete.

When onContentAfterDelete has a concrete Event subclass we will need to somehow use it. At the very least, the CoreEventAware trait does that for us. However, in Joomla 5 we won't have that trait since the EventAware trait which uses it will go away. Therefore in Joomla 4 we need to allow the developer to set that property to either an event name or an event class name. The logic would be simple:

  • Does a class with a name matching this property's value exist? If so, use it.
  • Does the CoreEventAware trait report that a concrete Event subclass exists for the event name equal to this property's value? Use that and log a deprecated notice.
  • Otherwise we assume it's a custom event name. We instantiate a GenericEvent object, assign it that event name, set its attributes and merrily go about our way firing that event.
    This will allow developers to write extensions which work on both Joomla 4 and Joomla 5, without fugly if-blocks, without headaches. For 99% of extensions which simply use the default event names as defined by the core *Model class' constructor the developers will be blissfully unaware of the underlying change. Come Joomla 5 all we will need to do is remove the compatibility layer which will be conveniently placed into a Trait. I like keeping things simple and maintainable.
avatar HLeithner
HLeithner - comment - 6 Mar 2022

@nikosdion can you add at least one event so it can be tested?

avatar HLeithner
HLeithner - comment - 6 Mar 2022

@nikosdion can you add at least one event so it can be tested?

avatar nikosdion
nikosdion - comment - 6 Mar 2022

@HLeithner I have already added support for the concrete event classes already added in Joomla 4. I think the GetIconEvent and DisplayEvent are easy targets for testing, no?

avatar HLeithner
HLeithner - comment - 6 Mar 2022

sorry my fault, I overlooked the GetIconEvent and didn't checked all events. Thanks that's enough of course.

avatar nikosdion
nikosdion - comment - 6 Mar 2022

No worries :) The list of events is loooooong!

avatar nikosdion nikosdion - change - 17 Mar 2022
Labels Added: ?
Removed: ?
avatar HLeithner
HLeithner - comment - 17 Mar 2022

Do we still need the namespace lookup in the future because it looks really expensive for something that get called several times within one request.

avatar HLeithner
HLeithner - comment - 17 Mar 2022
avatar nikosdion
nikosdion - comment - 17 Mar 2022

@HLeithner I would axe the old namespace lookup but the decision to do so is above my pay grade :) Say the word and I'll put that code on the chopping block.

I just fixed the docblock which was causing the CS error.

avatar HLeithner
HLeithner - comment - 17 Mar 2022

@HLeithner I would axe the old namespace lookup but the decision to do so is above my pay grade :) Say the word and I'll put that code on the chopping block.

I just fixed the docblock which was causing the CS error.

since this only works for core, because it uses the Joomla namespace we can drop it if we have all events in the new find event class function. I would change the ordering a bit of the create function. it's a bit strange that the subject check is between the class exist check.

correct me if I'm wrong.

avatar nikosdion
nikosdion - comment - 18 Mar 2022

@HLeithner I think that you are correct, that's exactly what I was thinking.

I would change the ordering a bit of the create function. it's a bit strange that the subject check is between the class exist check.

I would be a bit more provocative and say “are you sure we need the subject check outside the concrete event itself?”. I would argue that this check should be a Trait, used by every core event which expects a subject. In fact, since we do not check the subject parameter's type or value this functionality is already covered by the ReshapeArguments trait (it also checks for required named parameters). Would it not make more sense dropping this check and implementing the ReshapeArguments trait across all Joomla concrete events? IMHO the event class should be responsible for checking its arguments, not a god method.

avatar HLeithner
HLeithner - comment - 18 Mar 2022

I would be a bit more provocative and say “are you sure we need the subject check outside the concrete event itself?”. I would argue that this check should be a Trait, used by every core event which expects a subject. In fact, since we do not check the subject parameter's type or value this functionality is already covered by the ReshapeArguments trait (it also checks for required named parameters). Would it not make more sense dropping this check and implementing the ReshapeArguments trait across all Joomla concrete events? IMHO the event class should be responsible for checking its arguments, not a god method.

I'm not sure why a subject is required, especially since we have events with no argument. But that was before my time... On the other side this function is a utility function and shouldn't be used since you should create the event directly right?

I mean it doesn't do anything except including b/c code and mapping magic.

avatar nikosdion
nikosdion - comment - 18 Mar 2022

Correct, it's just a utility function, naively used in Table, View etc. Should we deprecate it for removal in 5.0? I don't see any point in keeping it there. I'd replace all calls to it with concrete events.

Also, sidebar. The way concrete events are now they are always constructed with two arguments, the event name and an array of parameters. IMHO this is dumb for two reasons:

  1. We have one concrete event per event name
  2. Having an array argument is opaque as to what arguments the event accepts which gives even less visibility into the events' inner workings than the old plugins.

I would very much prefer it if each concrete event's constructor accepted real arguments. For a real world implementation check what I've done in the under development version of LoginGuard (yup, I'm backporting some of this PR into my extension to make sure it's Real World Tested™).

Some possible snags with that:

  • Existing events (Table/, View/DisplayEvent, WebAsset/) would have to accept their first argument as string and second argument as array regardless of what they are supposed to do for the duration of Joomla 4 (therefore providing b/c). This should be deprecated with a removal target of 5.0. That's the same trick we've used in the past when changing method signatures.
  • There might be events which apply on multiple event names. In this case we need to standardise which argument should be the event name: the very first one or the very last one? My instinct says the very first one so we can add further arguments as needed.

Another point I observed yesterday. The getArguments() method does not return named arguments — and correctly so. This is not a problem with legacy listeners but it is a problem if you tried, like me, to use Joomla 4 events in 4.0 and 4.1. For example, doing something like [$foo, $bar] = $event->getArguments(); fails miserably because getArguments() returns null. The only way to fix that is to have the ReshapeArgumentsAware trait override getArguments() to return all named arguments made known to reshapeArguments. This would allow non-legacy listeners predating concrete event classes to still work.

Let me know of your thoughts and I'll probably be able to work on it later today.

avatar HLeithner
HLeithner - comment - 18 Mar 2022

On one side I'm completely with you fully typed parameters makes sense on the other side I like the idea of being more flexible to add information to an event.
Also if we override the __constructor with different parameters we can't abstract it anymore from a base Event class (which would be a problem for the AbstractImmutableEvent at the moment). I think it would also mean all event should to be final (maybe not a bad idea anyway).

On the other side if you have an array of arguments you can easily extend it without thinking about consequences for any abstraction. For example at the moment Joomla events are triggered very inconsistent with "context", sometimes it's the component it calls, sometimes it's the component it belongs too. So I would add 2 "new" contexts

  1. the extension the event belongs to ex. "com_fields.field"
  2. the context it's called in "com_content.article"

(The example is bad and it should be discussed in it's own issue) But I just wanted to demonstrate this could be simply added later when we have an array of arguments. But hard if we have 50 hardcoded constructors.

So I think it's better to stay with the array. ymmv

PS: I'm on the way to the cloudfest hackthon starting to work on tuf for joomla, so the next 2 hours I'm more responsive.

avatar nikosdion
nikosdion - comment - 18 Mar 2022

Also if we override the __constructor with different parameters we can't abstract it anymore from a base Event class (which would be a problem for the AbstractImmutableEvent at the moment)

Why? You can override the constructors just fine, even on PHP 8.1. This is what we are doing in all extension (component, plugin, module...) classes to inject dependencies from the service provider. This is the same thing I am doing in the code I linked you to.

The internal call to parent::__construct() can of course have the parameters the parent expects. Exactly like the aforementioned classes.

I think it would also mean all event should to be final (maybe not a bad idea anyway).

I agree 100% with you. The concrete classes are anyway extended from inheritable, abstract classes and composed of our various traits. There is no point for a developer trying to extend from our concrete events — or even for us extending one event from the other. Specifically for that final point, if we are ever tempted to do that we should slap ourselves in the face and use a trait at worst.

For example at the moment Joomla events are triggered very inconsistent with "context", sometimes it's the component it calls, sometimes it's the component it belongs too.

In both cases it's a string. The problem would be if sometimes it'd be a string and sometimes it'd be an object.

So I would add 2 "new" contexts

We can't and we shouldn't. This will cause massive breakage. The context for each event name has a specific semantic which is well-understood by developers. Changing that will break 17 years worth of code which will have to be rethought from scratch. If you want 3PDs to come after you with torched and pitchforks and kill adoption of Joomla 5 that's how you do it :p It will make FAR more sense to document what the hell $context refers to in the event constructor.

So I think it's better to stay with the array. ymmv

I disagree because it's an opaque obstacle for developers. If I do new SomeEvent($eventName, $arguments) as a developer new to Joomla I have two problems:

  • What the fsck should I put in $eventName?!
  • What the fsck should I put in $arguments?!

This is a horrid DX (Developer Experience). Even after 17 years I still run into this. What are the arguments for onContentPrepare? WTAF is the fourth ($page) argument when the context is not an article? Is the third ($params) argument mandatory if I don't have an article? Should the second ($item) parameter always be a dummy stdClass object or can I pass a concrete object without being afraid that some core or third party plugin will bork on PHP 9 when setting undefined properties will cause the code to throw an ErrorException? Why do I even have to think about that to run a simple event?!

The first problem is easy to overcome on our side by setting the constructor to public function __construct(string $eventName = 'onSomethingReallyCool', array $arguments = []);.

The second problem is impossible to overcome. Arbitrary dictionaries (arrays with string keys) are evil. PHP can't typehint dictionary "shapes" (define acceptable keys and their types). Therefore it moves the responsibility of key and type checking to the domain logic which can be very opaque, cannot be typehinted and is prone to many a bug. I am on a mission to remove them from my code (and ideally Joomla) exactly because I keep bumping into stupid issues like that!

Here's what a developer has to do now. We'd have to check where in the core this event is used. But this does not tell us about optional arguments. So we have to check all of the event handlers in the core. Again, this might not tell us about optional arguments either and if anything it confuses us even more as to how the event is meant to be used. We've spent half an hour poring through code and we're none the wiser. The hell? We just wanted to handle an event, not write a dissertation. Why not go with WordPress which documents everything in the Codex? And that's how Joomla loses developers to bad DX...

Sure, we could add a docblock but as experience tells us someone, somewhere will just start using an optional named parameter without changing the docblock and we're back to square one (and yes this has already happened with the legacy plugin events and it's driving me freaking nuts!).

Having named, typehinted, documented arguments in the final, concrete event classes would mean that:
a. third party developers don't need to guess (self-documenting code — no need for an external reference, like WordPress' Codex)
b. core developers MUST update the method signature and MUST document their changes; it's now become very easy to check AND enforce with our existing CI pipeline and PR workflow!
c. new named parameters go to the end and are optional, maintaining b/c (as opposed to magically adding a new mandatory argument, accidentally breaking b/c in a way that is very hard to impossible to prevent unless a cunning human code auditor spots the problem).
d. if point c is limiting you as a core developer it is a VERY STRONG hint that You Are Doing It Wrong™; you should be using a different event name, with its own, new concrete event class. This is super important because it effectively prevents clusterfuck events like onContentPrepare which is used for a gazillion different and conflicting purposes but isn't documented in full anywhere. Yes, that event is my nemesis. It's so messed up that I write perfectly working code only to find it broken by a 3PD plugin which didn't expect the data type I sent it — and neither of us did anything wrong, because this f!@#$% piece of sh$#% is not documented anywhere! ARGH!

I would also argue that the current concrete event implementation is relative useless because getArgument() basically returns mixed and I need to typehint everything in my code to have code completion ? We could use phpStorm's advanced metadata to properly typehint getArgument() on each concrete class, making the API discoverable, improving the DX of Joomla. Sounds like a small thing but if we want to attract more developers who will come to Joomla by choice (rather than being forced to) that's the way to do it.

These are all concepts and ideas I have been toying with the past decade in my now-deprecated framework. They never took the shape and form I wanted but they gave me a lot of experience designing and using a framework and identifying the DX pain points of an architecture.

I think you should mull it over before replying. Try to come up with a use case where you'd need to change the event constructor signature and ponder on whether it makes sense to keep the same event or come up with a new event. Ask Niels to do the same (I am pretty sure he already gets it) and give you feedback. You'll see how I arrived at my conclusion.

avatar HLeithner
HLeithner - comment - 18 Mar 2022

Ok, the constructor override is handled different in php as a "normal" function... missed that.

But what you propose is to kill the event system again and re-implement it new, is this the summary, at least the event class?
if you remove the setArgument / getArgument behavior then there isn't much left for event. That should have been done in j4.
I'm not sure if it is a good idea once again replace everything again... but @nibra can bring his opinion here.

Do you think of a way to make this b/c with 3.10 (style)/4.x and 5.0 event classes?

I'm not against doing it and I fully understand the benefits of it. But also the drawbacks.

Any way I think that this is unrelated of this PR right? We want to move to own Event classes for the moment, if we set the arguments later as array or parameters shouldn't effect that PR.

avatar nikosdion
nikosdion - comment - 18 Mar 2022

if you remove the setArgument / getArgument behavior then there isn't much left for event. That should have been done in j4.

But... I am not touching these. At all. Where did you get that idea?

Do you think of a way to make this b/c with 3.10 (style)/4.x and 5.0 event classes?

100% backwards compatible. That's what I was explaining. I have a way to make it fully b/c and forwards compatible and improve the DX. I had an epiphany yesterday — well, less than an epiphany as my adjusted dosage of medication finally kicked in and I'm back at 100% mental capacity.

Any way I think that this is unrelated of this PR right? We want to move to own Event classes for the moment, if we set the arguments later as array or parameters shouldn't effect that PR.

No, it has to be done now or never. If we have concrete event classes which do not have concrete, typehinted parameters we can never move into using them. Bear with me for a minute. Let's take the onGetIcon event, a very simple event, as an example.

  • OLD – Joomla 3 / 4.0 way: $event = new GenericEvent('onGetIcon, [$context]);`
  • INTERMEDIATE – PR up to this point way: $event = new GetIconEvent('onGetIcon', [$context]);
  • PROPOSED – Proposed way: $event = new GetIconEvent($context);

Going from OLD to either INTERMEDIATE or PROPOSED is fully b/c because we didn't have a concrete event before :trollface:

Going from INTERMEDIATE to PROPOSED, however, is a nightmare of mythical proportions. We'd have to change the constructor to:

/**
 * @param  string        $eventNameOrContext. The context or [DEPRECATED] the event name.
 * @param  array|null $argumentsOrSkip  [DEPRECATED] The event parameters if the previous parameter has the event name.
 *
 * @deprecated 5.0 The second parameter is deprecated. Pass the context to the first parameter.
/*
public function __construct($eventNameOrContext, ?array $argumentsOrSkip = null)

This is confusing and requires error-prone domain logic.

Meanwhile, this is the self-documenting proposed way:

public function __construct(string $context)

This is a simple event. With more complicated events, like onContentPrepare, it becomes even worse and the domain logic springs out of control. Multiply that times 50 or so core events and you have the kind of work NOBODY will do the next ten years.

It's far, FAR, FAR easier to go full steam ahead with the proposed way now, only having to deal with INTERMEDIATE to PROPOSED conversion for the 26 concrete event classes already present in Joomla 4.1. It's still a lot of trudge work but I'm willing to take one for the team and do it. Having to do it for 100 or so classes after 2–3 years? No. Nope. Nuh-uh. Hell to the nope.

Here's what. What if I do the work for GetIconEvent and the current 26 concrete classes in a branch for you to see what I'm talking about? Would that help?

avatar nikosdion
nikosdion - comment - 18 Mar 2022
avatar nikosdion
nikosdion - comment - 18 Mar 2022

I have updated the branch with a refactored Joomla\CMS\Event\View\DisplayEvent, including two specialised events (AfterDisplayEvent and BeforeDisplayEvent) to showcase how you can maintain full backwards compatibility while offering better DX.

If I have some more time tonight and tomorrow I will also do the same for the WebAsset, Workflow, Model and hopefully Table events. As you can see it's not that hard once you wrap your head around the way events are used and consumed (thankfully, I have done a LOT of work on this area the past year).

In short, yes, we can have our pie AND eat it too. If you like what you see on that branch I can kill off this PR and do a PR from the other branch — the other branch is already rebased on 4.2 since the ship for 4.1 has long sailed by now...

avatar HLeithner
HLeithner - comment - 19 Mar 2022

Sorry that you have to wait but I don't know when I have time this weekend to answer.

But before we change this signature, I would like talk to other maintainers to get an ok from them too.

avatar nikosdion
nikosdion - comment - 19 Mar 2022

OK, no problem. Just a heads up, the next couple of week I am unlikely to have any time — but at least I've done enough in my branch to be able to pick it up whenever / if you agree with this direction.

In the meantime I will have released the new version of LoginGuard with a backport of these ideas under my own namespace so I'll at least gain experience about the real world use of this code.

avatar laoneo
laoneo - comment - 23 Mar 2022

I didn't dig too deep into the code and did also not test it yet. Just to have a real world example from the plugin developer perspective who consumes the event. Will the following examples both work beside each other?

Old way:

public function onContentBeforeDisplay($context, &$row, &$params, $page = 0)
{
    return 'demo';
}

New way (Arguments are just an example, don't take the arguments for granted):

public static function getSubscribedEvents(): array
{
    return [ 'onContentBeforeDisplay' => 'onDisplay' ];
}

public function onDisplay(DisplayEvent $e)
{
    $e->setArgument('content', 'demo');
}
avatar nikosdion
nikosdion - comment - 23 Mar 2022

@laoneo The latter onDisplay event handler is wrong because there's no content argument in the event. Right now (Joomla 4.1.0) you would need to do this:

$result = $e->getArgument('result');
$result[] = 'demo';
$e->setArgument('result', $result);

This would work just fine with the PR, but it's a kludge :) So I added the addResult method which turns it back into an one-liner:

$e->addResult('demo');

Both of these approaches AS WELL AS the legacy one will work just fine side-by-side.

I would never break existing code. Legacy handlers will work just fine until the end of the 4.x release cycle. Since 4.0 let us handle real Events in a clumsy way we have to support that throughout the 4.x release cycle as well. Anything we add has to be built on top of legacy. That's the approach I am taking.

You will also see that in my branch (https://github.com/nikosdion/joomla-cms/tree/feature/event-redux-proposed) I am converting existing concrete events in a way which allows you to create them with BOTH the Joomla 4.0/4.1 way (passing eventname and an array of arguments) AND the new way (passing individual arguments). Since PHP doesn't offer method overloading for constructors (being a loosely typed language and all) I am faking it the same way we've faked it in the past, e.g. when we changed the method signatures for HTMLHelper::_('script').

Again, the objective is to put the foundations for the next, improved iteration of Events handling in core in a way that allows someone to write software which would be simultaneously compatible with Joomla 4 and 5, making upgrades possible, without breaking anything that already works on Joomla 4.

avatar laoneo
laoneo - comment - 23 Mar 2022

Then I'm more than happy! I would also add some @trigger_error('...', E_USER_DEPRECATED); statements when legacy is used, otherwise as extension developer there is no indication that a deprecated way is used.

avatar nikosdion
nikosdion - comment - 23 Mar 2022

@laoneo Fully agree! That's why I did exactly that 5 days ago: https://github.com/nikosdion/joomla-cms/blob/feature/event-redux-proposed/libraries/src/Event/View/DisplayEvent.php#L50-L56

I just want to get the green light from @HLeithner @nibra and the other leadership members so I can keep on converting existing event classes and introduce new ones. I think we've got the backwards and forwards compatibility covered :)

avatar HLeithner
HLeithner - comment - 23 Mar 2022

I just want to get the green light from @HLeithner @nibra and the other leadership members so I can keep on converting existing event classes and introduce new ones. I think we've got the backwards and forwards compatibility covered :)

Allon is here also because of the discussion in the maintainer chat. So it's ongoing^^

avatar nikosdion
nikosdion - comment - 23 Mar 2022

Awesome! This week is a great opportunity for the discussion to happen as I don't have time to work on the code anyway ?

avatar laoneo
laoneo - comment - 23 Mar 2022

I will give it tomorrow a test.

avatar laoneo
laoneo - comment - 24 Mar 2022

I have tested this item successfully on 0f9a041

Tested DPCalendar with the pr and can confirm that the old events are working the way I'm using them on Joomla 3.10. Also the icon one which uses the SubscriberInterface works as expected.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36578.

avatar laoneo laoneo - test_item - 24 Mar 2022 - Tested successfully
avatar HLeithner
HLeithner - comment - 24 Mar 2022

@nikosdion I had a discussion about events/messages and hooks/callbacks today and the future implementation of the event system in Joomla. Basically the idea is to split messages (Events like "onAfterExecute" where the result is not used) and callbacks (Events with result).

I would simple say adding an interface which describes this behavior is enough for the dispatcher to decide how to interact with the plugin. You more or less added this in https://github.com/joomla/joomla-cms/pull/36578/files#diff-58ec461e3704bfe1e12c23707a90c52718e1d37f2592976bc5256fb0b22f5c4fR56-R59 going a step further and explicit have an interface may would it make more clear.

The other thing is how events handle chained results. At the moment I get $event with the argument $item, since we want this immutable this object would only be clone and should be added with $this->addResult($item) to the result list. The question is now how would one plugin work on the $item of the previous plugin?

One more thing is the question of working more like a middleware and leverage the parameter and result validation to php instead of a ResultTypeXXXAware. I know it's possible todo more then check type, like range validation, the question is it really need? (I know we have "false" allow values and so).

avatar nikosdion
nikosdion - comment - 24 Mar 2022

Regarding the interface: I had already added the ResultAwareInterface. I am already using it in the GetIconsEvent: class GetIconsEvent extends AbstractImmutableEvent implements ResultAwareInterface. Isn't this what you are saying here?

The other thing is how events handle chained results. At the moment I get $event with the argument $item, since we want this immutable this object would only be clone and should be added with $this->addResult($item) to the result list. The question is now how would one plugin work on the $item of the previous plugin?

This would not be an immutable event :) In fact, this sounds like an event that acts on its input argument, e.g. onContentPrepare. This event would extends from AbstractEvent, NOT AbstractImmutableEvent. However, this has the obvious problem that all arguments can be mutated. We could have an interface PartiallyImmutableEvent and a Trait PartiallyImmutableEventAware where we explicitly set the mutable attributes. For onContentPrepare that would be both $item and $params.

For events which want all their input arguments immutable BUT would like to operate on the previous result, um, this is already possible since Joomla 4.0:

$previousResults = $event->getArgument('result', []);
// I am being stupid here and just assume the only result we wanna keep is the last one
$previousResult = array_pop($previousResults);
// Do something with $previousResult...
$previousResult->foo = 'bar';
// Replace the result
$event->setArgument('result', [$previousResult]);

Just note that this can only work with modern events. Legacy events do not have the notion of chained results. If you get a bit more specific about what event, existing or new, you want to implement I can tell you which approach would be better and why.

One more thing is the question of working more like a middleware and leverage the parameter and result validation to php instead of a ResultTypeXXXAware. I know it's possible todo more then check type, like range validation, the question is it really need? (I know we have "false" allow values and so).

Yes, it's really needed for two reasons:

  1. It would require reimplementing addResult on each event class.

  2. Overriding the addResult method with a typed parameter changes the method signature in a way that PHP 8.1+ complains about.

Therefore the alternative is to not implement addResult at all and have it implemented in each event separately which is very much not DRY. If someone has a way to implement it without adding a crapload of code in each event class I am open to suggestions. I figured that using a trait (one liner!) is best AND it leaves enough “breadcrumbs” for developers to understand how the magic works.

Also, as you already, said we do get a bonus with this. We can implement our own typeCheckResult method to add range checks, domain logic checks and everything else we need. I believe this will become particularly useful if we migrate towards Union types to replace dictionaries (key-value arrays) in a future version of Joomla. The logical way to do that is introduce another method (typeConvertResult) which would precede the call to typeCheckResult in the addResult method, convert legacy arrays to our Union type and then let a ResultTypeUnionAware trait typecheck and validate our Union type. Once you have this kind of middleware it becomes very easy modernising your code without breaking b/c. If you have PHP type checks you're stuck between a rock and a hard place, especially since PHP is moving to very strict method signature checks before it even considers method overloading (big mistake, in my opinion).

avatar HLeithner
HLeithner - comment - 1 May 2022

hey @nikosdion sorry I didn't comeback here earlier, Niels is not happy with it and I didn't liked to discuss this with him for the moment.

But now I came to this topic back and would like to know why we only do this with 'result', from my point of view it would be really limiting the system. But maybe it's only me.

Why we don't do somthing like

class event {
  function addArgument($name, $value) {
    $this->arguments[$name][] = $value;
  }
  function setArgument($name, $value) {
    $this->addArgument($name, $value); // we never override arguments
  }
  function getArgument($name) {
    return $this->arguments[$name][count($this->arguments[$name])-1];
  }
  function getArguments($name) {
    return $this->arguments[$name];
  }
  function getArgumentOriginal($name) {
    return $this->arguments[$name][0];
  }
}

That's complete pseudo code of course and all the checks have to be added (also the typeCheckResult traits). At least I see no reason to limit it only to the 'result' argument. Of course if we need it different for the result type because of b/c then we keep it only for this different? Or do I completely miss something? thanks.

avatar nikosdion
nikosdion - comment - 1 May 2022

The pseudocode you wrote is a mutable event. We already have that ?

The problem is, we want our events to be immutable (so the event handlers can't add, remove or change arguments) BUT for many of them we need to receive scalar results.

In an ideal world we'd have an immutable results argument which is a write-only queue. HOWEVER this would be backwards-INCOMPATIBLE. In Joomla 4 the results argument is a scalar array and handlers can reasonable expect to read its values as an array, add to the array or even completely replace the array. So, we cannot do it the ideal way.

This leaves us with the need to replace the scalar array result argument in otherwise immutable events. Hence the code I added.

In the end of the day, it comes down to this. Immutable events. Backwards compatibility. Doing it right. PICK TWO (2).

avatar HLeithner
HLeithner - comment - 11 May 2022

I know my feedback speed could be better... but other things came between me and joomla...
anyway last question then we can go I think.

We don't have a way to easily "chain" plugins right? What I mean is for example the onContentPrepareData event we set the $context and the $data (which could be mixed.... what a pain). in this case a plugin would work would like to work with $data.

(I know it's not a good example because the $data is modified directly by the plugin as reference, which I would expect is wrong per concept because we would like to validated the modification before the next plugin gets the new data right?)

function onContentPrepareData($event) {
  $data = $event->get('data');
  $data['myvalue'] = 'awesome';
  $event->addResult($data);
}

the next plugin would need a logic to get the data or the result ?

function onContentPrepareData($event) {
  $data = count($event->getResult()) > 0; $event->getResult() ? $event->get('data');
  $data['myvalue'] = 'awesome';
  $event->addResult($data);
}

Also in this case getResult is the wrong function name because I would get more then one results so it should be getResults?

The other thing is the context I would like to talk again with you but first finish this. topic.

avatar nikosdion
nikosdion - comment - 11 May 2022

The answer to this question predates any changes I have made and is part of the existing Joomla Framework Event package ?

When there are chained plugins we do not return a result. We modify the argument itself. This has always been the case in Joomla.

In this case the Event would be mutable, i.e. its arguments can be changed by any plugin in the chain. For onContentPrepareData the code would be:

$data = $event['data'];
$data['myvalue'] = 'awesome';
$event['data'] = $data;

Or you could also do:

$data = &$event['data'];
$data['myvalue'] = 'awesome';

The next plugin can access the data attribute.

(Side note: we could of course implement these events as immutable with JUST one argument being mutable, the same way my code uses immutable events with a mutable result parameter. This would be functionally equivalent to what Joomla does now in legacy event handlers, where only one argument is sent by-reference therefore mutable. We could even abstract that behaviour into a Trait.)

These events work kinda-sorta like middleware.

I say kinda-sorta because a plugin handler implicitly lets the execution flow to the next handler unless it calls $event->stopPropagation() whereas middleware would have to explicitly call the next middleware hander (or not, if it wants to stop the flow). Moreover, a Joomla event handler modifies the input argument instead of returning the modified argument. This is conceptually the opposite to the Middleware pattern but functionally identical — and keeps things beautifully backwards compatible.

avatar HLeithner
HLeithner - comment - 11 May 2022

Ok but in this case (how it is now) we don't validate anything and should get rid of it right? And the events which are immutable provide always the same content to all plugins and the caller have to work on all results. In this case it's not a middleware and more a messaging bus with a result value for each plugin.

Just to get it right. In the end we have 3 types of events.

  1. Immutable without result (messages)
  2. Immutable with a array of results (where all results could be overridden by each plugin as long as preventSetArgumentResult is false)
  3. Mutable where Sodom und Gomorra can do more or less everything as long as no validation is on in the Event class but the only way to do the middleware like interface
avatar nikosdion
nikosdion - comment - 11 May 2022

So, what I said is that you can have some kind of validation in the event itself IF you make it immutable AND use a bespoke setter for its only mutable argument.

However, also keep in mind that a lot of Joomla events, especially in the content family, pass around plain objects, typed objects and arrays like there's no tomorrow. What kind of validation are you doing in the event? It's impossible. That's why I never used generic catch-all-and-then-some events in FOF. It makes far more sense to have onComContentPrepareContent knowing that it expects a stdClass object with specific properties. That can be validated. What you have now, cannot. So, yeah, you're stuck with that in very generic events, they're just a stupid message bus and hope for the best.

Other events, however, pass specific object types. For example, all events handling queries. These CAN be worked into something approximating middleware.

Eventually, it'd make sense to migrate away from events using a context argument. If you need to pass a context it means you should have a different event name and type! Anyway, that's another fight for another day.

Just to get it right. In the end we have 3 types of events.

Immutable without result (messages)
Immutable with a array of results (where all results could be overridden by each plugin as long as preventSetArgumentResult is false)
Mutable where Sodom und Gomorra can do more or less everything as long as no validation is on in the Event class but the only way to do the middleware like interface

Correct.

The second case sounds like a bug but it's actually what Joomla 4 does right now. Eventually we'll move away from GenericEvent for them and implement them as concrete events with preventSetArgumentResult set to true. Then we can fix the Wild West that Joomla 4 introduced by accident ?

avatar HLeithner
HLeithner - comment - 11 May 2022

Ok thanks, I asked for a last feedback round from maintainers and if the pr is ready and you are ready and no further concerns are raised I would merge in one week.

avatar nikosdion
nikosdion - comment - 11 May 2022

Whoop! Whoop! Whoop! Thank you :)

avatar laoneo
laoneo - comment - 16 May 2022

Can extension devs also benefit from this pr as I see some hardcoded stuff for core events. Is there a way for 3rd party extension devs to register their names? Or do I miss here something?

avatar nikosdion
nikosdion - comment - 16 May 2022

Yup, you do miss the fact that extension developer can already create their own concrete event classes. I proved that in LoginGuard. See https://github.com/akeeba/loginguard/tree/development/component/backend/src/Event (which includes a backport of some of the code in this PR) and how I use these custom concrete events in plugins e.g. https://github.com/akeeba/loginguard/blob/development/plugins/loginguard/email/src/Extension/Email.php You can see how I call these events e.g. https://github.com/akeeba/loginguard/blob/development/component/backend/src/Controller/CaptiveController.php#L189

These are all possible since Joomla 4.0. The benefit of this PR to 3PDs is that concrete events can have a setResult method which validates data types (instead of being a dumb array) AND the custom event classes are ultra compact, e.g. https://github.com/akeeba/loginguard/blob/development/component/backend/src/Event/GetInfo.php

I actually wrote LoginGuard like that as a proof of concept of the utility of this PR in third party code.

avatar HLeithner HLeithner - change - 17 May 2022
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-05-17 17:37:26
Closed_By HLeithner
avatar HLeithner HLeithner - close - 17 May 2022
avatar HLeithner HLeithner - merge - 17 May 2022
avatar HLeithner
HLeithner - comment - 17 May 2022

I'm merging this without 2 tests because it's mainly a foundation PR which allow us to transform our events to the new systems and we would find errors while testing them.

@nikosdion thanks for this, it would be great if we can find some time (when we have a new programmers docu) to write a good documentation about the complete plugin system!

avatar nikosdion
nikosdion - comment - 17 May 2022

THANK YOU!

I will also amend my TFA PR (#37811) to use concrete event classes and show people how it's done.

I agree, we need better documentation for the plugin system. It's changed too much since Joomla 1.5, all documentation people can find is outdated. When to do that... That's a pained question. Can you find me an extra 6 hours a day? I could really use them ?

Add a Comment

Login with GitHub to post a comment