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

Work towards using concrete events everywhere maintaining b/c #36578

Merged
merged 14 commits into from
May 17, 2022
Merged

Work towards using concrete events everywhere maintaining b/c #36578

merged 14 commits into from
May 17, 2022

Conversation

nikosdion
Copy link
Contributor

@nikosdion nikosdion commented Jan 5, 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.

@HLeithner
Copy link
Member

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...

@nikosdion
Copy link
Contributor Author

  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 :)

@chmst chmst changed the base branch from 4.0-dev to 4.1-dev January 31, 2022 18:09
@nikosdion
Copy link
Contributor Author

@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.

Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

Sorry it did take so long...

I added some comments (mostly sugar/options).

One other thing reading the comment. The Joomla event system would move into "simple return values" for all events? so setArgument method is/should not longer used?

The rest looks pretty fine, for me. maybe we also get some feedback from @nibra on this. but I think it should be the way to go and if possible be included in 4.2, I think it's not worse the hassle to integrate it into patch releases even if it's full b/c.

libraries/src/Event/Result/ResultAwareInterface.php Outdated Show resolved Hide resolved
libraries/src/Event/Result/ResultAware.php Outdated Show resolved Hide resolved
libraries/src/Event/Result/ResultTypeObjectAware.php Outdated Show resolved Hide resolved
Nicholas K. Dionysopoulos and others added 2 commits March 4, 2022 10:42
Co-authored-by: Harald Leithner <leithner@itronic.at>
Co-authored-by: Harald Leithner <leithner@itronic.at>
@nikosdion
Copy link
Contributor Author

@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.

@HLeithner
Copy link
Member

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.

@roland-d
Copy link
Contributor

roland-d commented Mar 4, 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.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester Unit/System Tests labels Mar 4, 2022
@nikosdion nikosdion changed the base branch from 4.1-dev to 4.2-dev March 4, 2022 20:47
@nikosdion nikosdion marked this pull request as ready for review March 4, 2022 20:47
@joomla-cms-bot joomla-cms-bot added PR-4.2-dev and removed Unit/System Tests NPM Resource Changed This Pull Request can't be tested by Patchtester labels Mar 4, 2022
@Quy Quy removed the PR-4.1-dev label Mar 4, 2022
@nikosdion
Copy link
Contributor Author

@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.

@HLeithner
Copy link
Member

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

1 similar comment
@HLeithner
Copy link
Member

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

@nikosdion
Copy link
Contributor Author

@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?

@HLeithner
Copy link
Member

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

@HLeithner
Copy link
Member

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.

@nikosdion
Copy link
Contributor Author

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.

@laoneo
Copy link
Member

laoneo commented Mar 23, 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');
}

@nikosdion
Copy link
Contributor Author

@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.

@laoneo
Copy link
Member

laoneo commented Mar 23, 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.

@nikosdion
Copy link
Contributor Author

@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 :)

@HLeithner
Copy link
Member

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^^

@nikosdion
Copy link
Contributor Author

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

@laoneo
Copy link
Member

laoneo commented Mar 23, 2022

I will give it tomorrow a test.

@laoneo
Copy link
Member

laoneo commented Mar 24, 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.

@HLeithner
Copy link
Member

@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).

@nikosdion
Copy link
Contributor Author

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).

@HLeithner
Copy link
Member

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.

@nikosdion
Copy link
Contributor Author

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).

@HLeithner
Copy link
Member

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.

@nikosdion
Copy link
Contributor Author

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.

@HLeithner
Copy link
Member

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

@nikosdion
Copy link
Contributor Author

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 😉

@HLeithner
Copy link
Member

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.

@nikosdion
Copy link
Contributor Author

Whoop! Whoop! Whoop! Thank you :)

@laoneo
Copy link
Member

laoneo commented May 16, 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?

@nikosdion
Copy link
Contributor Author

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.

@HLeithner HLeithner merged commit 63b0744 into joomla:4.2-dev May 17, 2022
@HLeithner
Copy link
Member

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!

@nikosdion
Copy link
Contributor Author

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 🤣

roland-d pushed a commit that referenced this pull request Jun 4, 2022
)

* Captive TFA

Import YubiKey plugin

* Captive TFA

Prepare SQL for new plugins

* Captive TFA

Import Fixed plugin (EXAMPLE)

* Captive TFA

System plugin

* Captive TFA

Replace the two factor authentication integration in the core

* Captive TFA

Fix wrong SQL / table name

* Captive TFA

Use correct prefix in the TFA helper when getting config UI

* Captive TFA

Fix a whoopsie or four

* Captive TFA

Coffee has long stopped working

* Captive TFA

Format the Methods page

* Captive TFA

Fix wrong TFA method internal name

* Captive TFA

Make sure we get the right view in the controllers

* Captive TFA

Remove yet another integration of the legacy TFA

* Captive TFA

Automatic migration from old TFA upon first login

* Captive TFA

Frontend MVC

* Captive TFA

Frontend routing

* Captive TFA

Style the method select page

* Captive TFA

Missed a legacy integration which needs removal

* Captive TFA

Better format of the configuration UI in the profile page

* Captive TFA

Use language strings when migrating data from legacy TFA

* Captive TFA

Only show the prompt to add a TFA method if none is already added

* Captive TFA

YubiKey should allow entry batching

This means that you can authenticate with any registered
YubiKey in your user profile.

* Captive TFA

Replace Tfa::triggerEvent

* Captive TFA

Import WebAuthn plugin

* Captive TFA

Improve TFA behavior on non-HTML pages. Basically, block
them!

* Captive TFA

Replace alerts with Joomla messages

* Captive TFA

Move onUserAfterDelete code to the `joomla` user plugin

* Captive TFA

Remove the System - Two Factor Authentication plugin

Use a trait for the application and fold the rest of
the code into Joomla's core user plugin.

* Captive TFA

Remove accidental leftover references to loginguard

* Captive TFA

Import Code by Email plugin

* Captive TFA

Post-installation messages

* Captive TFA

Enable the TFA plugins on NEW installations

* Captive TFA

XML formatting

* Captive TFA

Language and grammar in comments

* Captive TFA

Rearrange XML attributes

* Captive TFA

Fix typo

* Captive TFA

Fix wrong language key name

* Captive TFA

Remove leftover legacy TFA options

* Captive TFA

Fix wrong CSS class

* Captive TFA

Merge the padding classes

* Captive TFA

This lang string should never have had a link

* Captive TFA

Hide the Key emoji from screen readers

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Use “Two Factor Authentication” / TFA consistently

* Captive TFA

Tytytytypo

* Captive TFA

Fixed PHPCS issue unrelated to PR but reported by Drone nonetheless

* Captive TFA

Lang improvement

Co-authored-by: Brian Teeman <brian@teeman.net>

* Captive TFA

Lang improvement

Co-authored-by: Brian Teeman <brian@teeman.net>

* Captive TFA

Remove no longer valid plugin options

* Captive TFA

Typo in plugin path

* Captive TFA

Move TFA options in com_users config next to the
password options

* Captive TFA

Add Show Inline Help button to com_users' options page

* Captive TFA

Move loading static assets to the view template

See #37356 for
the reasoning. This should REALLY have been documented
somewhere...

* Captive TFA

Fixed wrong plugin path

* Captive TFA

Language style guide

Co-authored-by: Brian Teeman <brian@teeman.net>

* Captive TFA

Language style guide

* SQL code style and consistency fixes

* Add "CAN FAIL" installer hint

* Change longtext to mediumtext

* Change longtext to mediumtext in update script

* No default value for method

* Use real null values for last_used

* Captive TFA

Fix JS linter errors

* Captive TFA

Fix PHPCS issues after merging @richard67 's PR

* Captive TFA

Update formatRelative to use JNEVER, simplifying the
code in the view templates.

* Captive TFA

Fix typo

* Captive TFA

Fix transcription error

* Captive TFA

Show correct TFA column in the backend Users page

* Captive TFA

Fix PHPCS errors in UsersModel unrelated to this PR

* Captive TFA

Add note about supported browsers in TOTP's link

* Captive TFA

Remove bogus ESLint notice about qrcode

* Captive TFA

Fix confusing prompt

* Captive TFA

Consistently change ->qn to ->quoteName

* Captive TFA

Strict equality check

* Captive TFA

Move setSiteTemplateStyle to the views

* Captive TFA

Rename regbackupcodes to regenerateBackupCodes

* Captive TFA

Rename dontshowthisagain to doNotShowThisAgain

* Captive TFA

Throw deprecated notices from deprecated methods

* Captive TFA

Strict comparison

* Captive TFA

Typo in comment

* Captive TFA

Rename TwoFactorAuthenticationAware to TwoFactorAuthenticationHandler

* Captive TFA

Fix comment typo

* Captive TFA

Remove variables from SQL when not necessary

* codestyle changes

* Renamed SiteTemplateAware to SiteTemplateTrait

Change made against feedback item #37811 (review) for pull request #37811 with title "Improved Two Factor Authentication".

Feedback item marked the SiteTemplateAware trait name and had the following content:

> Still ...Aware is not a good name for a trait, since it usually denotes interfaces

* Remove more instances of "2SV"

Per feedback item #37811 (comment)

* s/Two Step Verification/Two Step Validation/

Per feedback item #37811 (comment)

* Language style

Per feedback item #37811 (comment)

* Remove unnecessary language string

* Remove redundant paragraph tags from PLG_TWOFACTORAUTH_EMAIL_XML_DESCRIPTION

Per feedback item #37811 (comment)

* Remove redundant paragraph tags from PLG_TWOFACTORAUTH_EMAIL_XML_DESCRIPTION

Per feedback item #37811 (comment)

The other file with the same language string I forgot to put in the previous commit.

* Remove the info tooltip in the methods list

Addresses feedback in #37811 (comment)

* Simplify the TFA enabled / disabled message

Per feedback item #37811 (comment)

* Fix layout of backup codes in methods list

Per feedback item #37811 (comment)

* Fix mail message

Per #37811 (comment)

* Confirm TFA method deletion

Per feedback item #37811 (comment)

* Simplify code label in Email plugin

Per feedback #37811 (comment)

We show short instructions above the field and the field label is simplified. Applied the same change to the Fixed plugin for consistency.

* Remove more dead code referencing the legacy TFA

* Use concrete events

This was the plan all along. Now that #36578 is merged we can FINALLY do it!

* WebAuthn support for some Android devices and FIDO keys

Backported from #37675

* Rename Tfa to Mfa

Ongoing process

* Move Joomla\CMS\Event\TwoFactor to Joomla\CMS\Event\MultiFactor

Ongoing process

* Two Factor Authentication => Multi-factor Authentication

Ongoing process

* `#__user_tfa` => `#__user_mfa`

Ongoing process

* twofactorauth => multifactorauth

Ongoing process

* Change the post-install message

Ongoing process

* Remove references to “second factor”

Ongoing process

* Remove the legacy TFA plugins

* I missed a few things

* I missed a few more things

* Wrong redirection from post-installation messages

Addresses #37811 (comment)

* Fix NotifyActionLog expected event names

Addresses feedback item #37811 (comment)

* Improve display of Last Used date

Addresses feedback item #37811 (comment)

* MFA extension helper

moves the group to the correct alpha order in the array now that it doesnt begin with T

* Remove unused field

* Remove no longer used language strings

* Undo changes in old SQL scripts

* Improve layout and accessibility of the methods list page

Based on VoiceOver testing on macOS 12.4 and the feedback from #37811 (comment) and #37811 (comment)

* Add missing options to plg_multifactorauth_email

* Sort lines alphabetically

Why not confuse the translators with out of order labels providing zero context to what they are translating? It's the One True Joomla Way...

* Add label to the One Time Emergency Password input

Per feedback item #37811 (comment)

* Sort lines

* Fix PHPCS complaint

* Formatting of XML files

* Forgot to remove extra CSS class

* Apply suggestions from code review

Formatting, wrong copyright and version information

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update build/media_source/plg_multifactorauth_webauthn/js/webauthn.es6.js

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Fix update SQL

Feedback item #37811 (review)

* Onboarding would result in a PHP exception

Feedback item #37811 (comment)

* Make MFA plugins' publish state consistent between MySQL and PostgreSQL

Feedback item #37811 (review)

* Update administrator/components/com_users/src/Controller/MethodsController.php

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/mysql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/mysql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.2.0-2022-05-15.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

* Restore obsolete language strings

Per discussion with @bembelimen

I had to rename One Time Emergency Passwords to Backup Codes so as not to make major changes to the obsolete language strings. Having them named One Time Emergency Passwords (OTEPs) was both misleading (they are not passwords, they are second factor authentication codes) and would collide with the `_OTEP_` component of language existing strings. Backup Codes is a good compromise, one that is also field tested for nearly seven years. So, there you go!

* Re-add the obsolete plugins' language files

Per discussion with @bembelimen

Yes, it's pointless, it looks wrong, it is what it is. At least I've put a header that this file needs to be removed.

* Remove no longer used twofactor field

* Rename CSS class to com-users-profile__multifactor

* Update administrator/language/en-GB/plg_multifactorauth_email.sys.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/plg_multifactorauth_email.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/plg_multifactorauth_email.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Accessibility improvement

* Improve language

* Change the heading level

* Fix case of extension registry file

Regression after renaming TFA to MFA

* Remove accidental double space after echo

* Remove BS3 leftovers

* Remove BS3 leftovers

* Remove BS3 leftovers

* Update administrator/components/com_users/tmpl/methods/list.php

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update components/com_users/tmpl/methods/list.php

Co-authored-by: Brian Teeman <brian@teeman.net>

* PHP warnings when there are no MFA plugins enabled

* MFA onboarding was shown with no MFA plugins enabled

* Backup codes alert is narrower than page on super-wide screens

* Backup codes alert heading font size fix in backend

* Revert wording for JENFORCE_2FA_REDIRECT_MESSAGE

* Backend users without `core.manage` on com_users were blocked

They were blocked from setting up / manage their on MFA,
blocked from the onboarding page and blocked from the
captive login page.

* Onboarding in backend shouldn't have a Back button

* Improve layout of method add/edit page

* Remove unnecessary H5 tag from TOTP setup table

* Kill that bloody Back button with fire

* MFA WebAuthn: use Joomla.Text instead of Joomla.JText

* MFA WebAuthn: show meaningful error on HTTP

* MFA Email: more sensible email body

* MFA WebAuthn: must be able to edit the title

* MFA add/edit: remove placeholders, replace with help text

* Heading levels

We assume an H1 will already be output on the page. This is always true on Atum and never true on Cassiopeia — but very likely on real world sites's frontend templates. So it's a compromise which is at least better than the previous case of starting at h3 or h4.

* Editing a user would show the wrong interface

When editing a user other than ourselves we need to show the MFA editing interface for the user being edited, not the MFA editing interface for our own user.

* Refactor security checks

Now they are conforming to the original intention

* Add missing Group By to the SQL query

* Show MFA enabled when a legacy method is enabled

* Users: filter by MFA status

* Language clarification

* Move the frontend onboarding page header to the top

* User Options language clarification

* PostgreSQL installation SQL wasn't updated

* Adding periods to the end of lines of error messages you will never, ever see

* Remove a tab

* Remove another tab from a comment

* Typo removing junk

* Remove useless imports

* Busywork

* Typo in the INI file

* Align comment

* Remove redundant SQL for PostgreSQL

* Typo in labels' `for` attribute

* Move backup codes to the top of the page

* Mandatory and forbidden MFA was not taken into account

If only one group matched, due to typo.

* Show information when MFA is mandatory

* Make the buttons smaller

* The secondary button looks horrid in the frontend

* Redirect users to login page in the frontend

When they try to access a captive or methods / method page.

* MFA Email: fallback to standard mailer when the mail template isn't installed

* Delete backup codes when the last MFA method is deleted

* Use text inputs for TOTP

With the correct input box attributes

* Fix the buttons for WebAuthn

* Clarify language strings

* Use toolbar buttons in the backend

Except for screen size small and extra small. Over there we ALSO display the inline content buttons because the toolbar buttons are hidden behind an unintuitive gears icon.

JUST BECAUSE THE DEFAULT JOOMLA WAY IS TO USE A TOOLBAR IT DOES NOT MEAN THAT IT MAKES SENSE ALWAYS, EVERYWHERE. THE USER IS KING. WE SERVE THE USER, NOT OURSELVES!

* Change the icon classes

* Forgot to copy over the changes to the frontend

* Regression: configure existing authenticators

We used to set field_type to custom to make the code entry disappear. After the changes to the field type handling we need to instead set input_type to hidden.

* Backup codes should never become the default method automatically

* Improve methods list layout

Now it is more clear which methods are enabled and which are available.

* Use toolbar buttons in backend pages

Except when the screen size is extra small which is the point where the toolbar is hidden and the interface becomes unintuitive.

* Fix return URLs for backend MFA edit pages

* Edit / Delete buttons mention the auth method name in the respective button's visually hidden text

* RTL aware back buttons

* Consistent use of the term Fixed Code

* Fix typo

Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>
@HLeithner HLeithner mentioned this pull request May 2, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants