Feature PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
2 May 2023

Summary of Changes

Add classes for Application events.

Side effect: with this we do not need to inject application in to every plugin.

Testing Instructions

Apply patch, make usre al works as before.

Addittionaly, add to any system plugin any event, eg:

public function onAfterDispatch(Joomla\CMS\Event\Application\AfterDispatchEvent $event)
{
        dump($event->getApplication()->getName());
}

Actual result BEFORE applying this Pull Request

All works.

Expected result AFTER applying this Pull Request

All works.
Addittionaly the dump output will show application name: site or administrator.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org: https://docs.joomla.org/Plugin/Events#System
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org: IDK
  • No documentation changes for manual.joomla.org needed
avatar joomla-cms-bot joomla-cms-bot - change - 2 May 2023
Category Libraries Front End Plugins
avatar Fedik Fedik - open - 2 May 2023
avatar Fedik Fedik - change - 2 May 2023
Status New Pending
avatar HLeithner
HLeithner - comment - 2 May 2023

If I'm not wrong we directly call the Event and not the AbstractEvent::create function?
@nikosdion would you be so kind and have a look since you started this with #36578

avatar Fedik
Fedik - comment - 2 May 2023

We can do new PotatoEvent('onFoobar') or AbstractEvent::create('onFoobar'). They both is fine.
Last one is a bit more flexible, example when need to refactor/change event class you change it in one place instead of replacing all occurrence.

I am fine with anything.

UPD. I think in extensions it will be better do direct new ExtensionPotatoEvent('onFoobar') since they will have own events which is not core one. Example in Finder Indexing events.

avatar nikosdion
nikosdion - comment - 2 May 2023

TL;DR: Changes required. See below.

So, in #36578 we introduced the concept of concrete event classes for all the things, with the asterisk that old stuff without a concrete event class should get its own concrete class. We also said that from now on we should be instantiating the concrete event class instead of going through AbstractEvent::create().

To make sure that legacy code would not break we introduced the CoreEventAware trait and modified AbstractEvent::create() to go through CoreEventAware ::getEventClassByEventName(). If the event name passed to AbstractEvent::create() is found in the hardcoded internal mapping of CoreEventAware then an event object of the correct concrete event class is created.

Since Fedik updated the magic mapping trait (CoreEventAware) we introduced in #36578 his PR does work fine.

However, since we had decided to move away from the opaque magic of AbstractEvent::create(), all these calls in code code should be replaced with direct instantiation of the concrete event class.

Example:

DO this $event = new \Joomla\CMS\Event\Application\AfterInitialiseEvent('onAfterInitialise')

DO NOT do this $event = AbstractEvent::create('onAfterInitialise')

Better yet, do this:

\Joomla\CMS\Event\Application\AfterInitialiseEvent()

by having a default value for the event name on all new events. This is important for Joomla 5.0 and later which requires PHP 8.1 or later. If $name has a default value you can do things like this:

$event = new SomeEventWithoutAnyArguments();

$event = new SomeEventWithArguments(arguments: [
  'foo' => $foo,
  'bar' => $bar
]);

That is to say, we will not have to type the event names ever again. Just using their concrete class will suffice.

avatar Fedik
Fedik - comment - 2 May 2023

Hm hm, I like the magick ?‍♂️
Originaly I wanted to do direct event, but started with create.

Well, AbstractEvent::create also have a benefits, example when we decide to use another class for the event, then no need to edit much.

Okay, then we probably should deprecate AbstractEvent::create method and CoreEventAware trait,
to be removed in j 6 or 7 :)
Because someone else will start to use it in new code.

avatar Fedik
Fedik - comment - 2 May 2023

I see the original PR for AbstractEvent::create was also from you e9c1efc 7 years old already :)

avatar nikosdion
nikosdion - comment - 2 May 2023

@Fedik Yes, the original concept back in August 2015 was to replace the Observer/Observable with a real event system which, at the time, was just fresh off the Framework repo. I was tasked with making it happen. The requirement at this very early stage of pre-alpha planning was to get something working so we can evaluate the new solution. This was meant to be a temporary shim.

The second part of the plugins-as-Events idea was to have concrete event classes for all events. Unfortunately, for the reasons you know…

(achoo-Joomla-X-achoo… oof, darned seasonal allergies!)

…this was not carried out before Joomla 4 was released. So, the temporary solution persisted. Derp!

#36578 was the attempt to introduce that second part, even if it was anywhere between 2 to 6 years too late, to FINALLY get Joomla events to concrete event classes.

As for this:

Well, AbstractEvent::create also have a benefits, example when we decide to use another class for the event, then no need to edit much.

This is why we have class aliases and the exact reason we will be having a b/c plugin in future Joomla versions.

As with all classes which have been renamed, changed namespaces, and generally stirred and shaken, this is easy to fix and absolutely not a reason to stick with a bad architecture.

Bad architecture? Yes, BAD architecture. Absolutely.

All these static calls? All these look up arrays? All that bollocking before we can actually dispatch an event (see triggerEvent, another TEMPORARY solution I introduced and Michael refined, originally meant to be removed in J5)? All that is wasted CPU cycles. On something we call literally HUNDREDS of times per request. Once we get rid of that you can expect another dramatic speedup of the CMS.

The last pièce de résistance was the fugly instantiation conventions of concrete event classes. For example, typing new SomeEvent('onSomeEvent', ['foo' => $bar, 'baz' => $bat]) is phenomenally ugly. You can't move the parameters around because legacy and forwards compatibility with PHP 8. However, once Joomla itself requires PHP 8 this is no longer an issue as long as the event name is the default value of the parameter because, as I said, we can simply write:

$event = new SomeEvent(arguments: ['foo' => $bar, 'baz' => $bat]);

And you know what else?

In the future, we can get rid of $arguments without killing b/c. Shocker, innit?!

How would that magic work. Ah, very simple!

Let's say the old Joomla 5 signature of the concrete event is:

__construct($name = 'onWhatever', array $arguments[])

and let's say it accepts parameters $foo and $baz of type string and object respectively. You can then change this signature to the following in Joomla 6:

__construct($name = 'onWhatever', array $arguments[], string $foo, object $baz)

So now you can instantiate the event either the old way:

$event = new SomeEvent(arguments: ['foo' => $bar, 'baz' => $bat]);

Or the new way:

$event = new SomeEvent(foo: $bar, baz: $bat);

Then in Joomla 7 you can simply drop $name and $arguments from the signature:

__construct(string $foo, object $baz)

and the Joomla 6 way of instantiating the event still works. Magic!

Who knows, maybe PHP 9 will give us even better tools. I definitely didn't have the PHP 8 tools back in 2015. But just because I was hamstrung by the need of b/c and the limitations of the language 7 years ago doesn't mean that you should write code hamstrung by the same no longer existing limitations and force this crap upon developers 8 years into the future. There's no reason to suffer in 2030 because 15 years earlier PHP 7 didn't give us the tools to change the method signatures without mucking up b/c.

Write code for the future, not the past.

avatar Fedik
Fedik - comment - 6 May 2023

Okay guys, next question, should we place events
under Joomla\CMS\Event\Application\FoobarEvent (under src\Event\Application) as it is now,
or
under Joomla\CMS\Application\Event\FoobarEvent (under src\Application\Event)

Second one looks more logical to me, in the app class can just do new Event\FoobarEvent (without bunch of imports).
I thinking about to change to second one.
Thoughts?

Generic events (like onContentPrepare etc) will stay under src\Event, but for Application I think it would make sense to keep them within Application namespace.

avatar nikosdion
nikosdion - comment - 6 May 2023

Since all other concrete events have been added under Joomla\CMS\Event it makes sense to continue with the same convention for Joomla\CMS\Event\Application. This will make it easier for third party developers to locate the concrete events.

Once all events are concrete we could distribute them to the respective extension package and add class aliases.

Using Joomla\CMS\Application\Event is premature right now and it would lead to confusion due to the inconsistency of the convention already used in Joomla 4. Don't catch developers off guard; they cannot possibly monitor all PRs, they cannot read your mind. Give them consistent conventions and some breadcrumbs to follow in order to deduce said convention.

avatar Fedik
Fedik - comment - 6 May 2023

Okay, then I will keep under Joomla\CMS\Event

avatar nikosdion
nikosdion - comment - 6 May 2023

Thank you ??

avatar Fedik Fedik - change - 6 May 2023
Labels Added: Feature PR-5.0-dev
avatar Fedik
Fedik - comment - 6 May 2023

PR is updated, now it uses new FooBarEvent instead of create() thing.

avatar laoneo laoneo - test_item - 11 Jun 2023 - Tested successfully
avatar laoneo
laoneo - comment - 11 Jun 2023

I have tested this item successfully on 2083a8e

Tested the onBeforeCompileHead in the accessebility plugin event with the old signature and the concrete class.


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

avatar HLeithner
HLeithner - comment - 21 Jul 2023

@Fedik can you solve the merge conflicts please then I merge this before a3

avatar Fedik
Fedik - comment - 21 Jul 2023

Hmhm, this should go first and then #40512 not vice versa ?
But yea I will fiix,

avatar HLeithner
HLeithner - comment - 21 Jul 2023

I noticed it and fixed your merge conflict in the meanwhile sorry

avatar HLeithner HLeithner - close - 21 Jul 2023
avatar HLeithner HLeithner - merge - 21 Jul 2023
avatar HLeithner HLeithner - change - 21 Jul 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-07-21 12:28:51
Closed_By HLeithner
avatar HLeithner
HLeithner - comment - 21 Jul 2023

thanks, I think we also need some migration documentation for this on manual.joomla.org

Add a Comment

Login with GitHub to post a comment