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

[5.0] Application event classes #40522

Merged
merged 19 commits into from
Jul 21, 2023
Merged

[5.0] Application event classes #40522

merged 19 commits into from
Jul 21, 2023

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented May 2, 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

@HLeithner
Copy link
Member

HLeithner commented May 2, 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

@Fedik
Copy link
Member Author

Fedik commented May 2, 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.

@nikosdion
Copy link
Contributor

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.

@Fedik
Copy link
Member Author

Fedik commented May 2, 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.

@Fedik
Copy link
Member Author

Fedik commented May 2, 2023

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

@nikosdion
Copy link
Contributor

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

@Fedik
Copy link
Member Author

Fedik commented May 6, 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.

@nikosdion
Copy link
Contributor

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.

@Fedik
Copy link
Member Author

Fedik commented May 6, 2023

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

@nikosdion
Copy link
Contributor

Thank you 🙏🏽

@Fedik
Copy link
Member Author

Fedik commented May 6, 2023

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

 Conflicts:
	libraries/src/Event/CoreEventAware.php
@laoneo
Copy link
Member

laoneo commented Jun 11, 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.

 Conflicts:
	libraries/src/Event/AbstractEvent.php
@HLeithner
Copy link
Member

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

# Conflicts:
#	libraries/src/Application/AdministratorApplication.php
#	libraries/src/Application/ApiApplication.php
#	libraries/src/Application/SiteApplication.php
@Fedik
Copy link
Member Author

Fedik commented Jul 21, 2023

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

@HLeithner
Copy link
Member

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

@HLeithner HLeithner merged commit e7e1fb4 into joomla:5.0-dev Jul 21, 2023
@HLeithner
Copy link
Member

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

@Fedik Fedik deleted the app-events branch July 21, 2023 12:47
GeraintEdwards pushed a commit to GeraintEdwards/joomla-cms that referenced this pull request Aug 14, 2023
* App events: create

* App events: base app event

* App events: add couple event classes

* App events: doc event

* App events: organize

* App events, couple more events

* App events, formatting

* App events, more events

* App events, doc to app

* App events, use same dispatcher

* App events, create to new

---------

Co-authored-by: Harald Leithner <[email protected]>
GeraintEdwards pushed a commit to GeraintEdwards/joomla-cms that referenced this pull request Aug 14, 2023
* App events: create

* App events: base app event

* App events: add couple event classes

* App events: doc event

* App events: organize

* App events, couple more events

* App events, formatting

* App events, more events

* App events, doc to app

* App events, use same dispatcher

* App events, create to new

---------

Co-authored-by: Harald Leithner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants