Validate events

[HLeithner]

Validate input and make the events immutable

[laoneo]

Why immutable?

[HLeithner]

Most of the CMS Plugins are immutable except tables (also suggested in the documentation) and there is no reason to modify the event beside the allowed arguments.

[Fedik]

@HLeithner you know I will not like it 😉

I think it a bit to much. I would validate only "entry" level, not that deep.
Ideally each value should be an Object instance of something (call it subject), so you just validate the instance. I do not think it really critical for URL example (however it can be instance of JUri).

Additionally, in current case the event work like a "hook", it should be allowed to modify url and files, other way whole events does not make any sense.

joomla-cms/libraries/src/Event/AbstractEvent.php

Lines 27 to 34 in 3a5984d

	*
	* You can access the subject object from your event Listener using $event['subject']. It is up to your listener to
	* determine whether it should apply its functionality against the subject.
	*
	* This AbstractEvent class implements a mutable event which is allowed to change its arguments at runtime. This is
	* generally unadvisable. It's best to use AbstractImmutableEvent instead and constrict all your interaction to the
	* subject class.
	*

About "imutable", it only means you cannot add/remove event arguments (with unset($event['subject'])), however you still able to modify the "subject state".
No cloning please.

I would reset all changes back to 47dd968 commit, and just add validation when setArgument is called, like I sugested in comment
joomla#35396 (comment)

[Fedik]

Even more.

After reading that comment again, I think we made mistake.
And should drop getUrl(), getFiles(), the event is array access object.
All this should be set as subject argument for each event:

public function __construct($name, array $arguments = array())
{
	parent::__construct($name, $arguments);

	// Check for required arguments
	if (empty($arguments['subject']) ... and type validation)
	{
		throw new \BadMethodCallException("Argument 'subject' of event $name is not of the expected type");
	}
}

For Url the subject is URL, for File(s) the subject is File object, and Array of files (for multiple)

[HLeithner]

The reason is why I validate the file object is because the file object has not a proper interfaces and class defined that should validate the input. I don't know why doesn't have it's own interface maybe creating one is a better idea but I would expect that could and in a b/c... which is not covered by our b/c policy because it's not in the library... hmm...

Any way we as the cms have to see anything that comes from a plugin as possible broken and must validate it. If we don't do it in the plugin we have to do it in the model. Which is the wrong position because each plugin which is called needs a proper state and if one plugin fucks it up it should instantly trigger an error.

The complete concept about the plugin system (using setArgument that calls setXYZ function) is to make a proper validation. And the validation I implemented is very weak.

Of course it doesn't matter if you call it subject or file or files or url so it's ok to rename it to subject. But still I see no reason to allow more modifications then necessary and that's true for the complete cms and framework. Every time we allow people to modify something we have to deal with it as possible b/c break if we change it. And this makes it much harder to introduce new features or modify/optimize existing code. Also testing (I know we lack on testing) would be in a much easier if we only need to test what's needed/allowed.

[Fedik]

Okay then. Only a couple notes :)

I understood what do you mean, but it not a place to do full object validation.
Because the File object comes from AdapterInterface as stdClass, and it already can come with any set of properties, so it may be broken already at that stage.

We have to live with it, because bc.

Hmhm, I see that ApiModel checking only for $file->type, $file->path, $file->content, $file->adapter. If you really want, then it is enough to validate only these, I think. And stuff like width/height is not applicable for pdf or other documents.

(And maybe we have to merge File and Files event to single, and use only Files, they basically the same. But can stay as is)

In general it is okay.

[HLeithner]

actually it's not a b/c because we have a documentation for the basic values it's defined in the AdapterInterface

	 * - type:          The type can be file or dir
	 * - name:          The name of the file
	 * - path:          The relative path to the root
	 * - extension:     The file extension
	 * - size:          The size of the file
	 * - create_date:   The date created
	 * - modified_date: The date modified
	 * - mime_type:     The mime type
	 * - width:         The width, when available
	 * - height:        The height, when available

(And maybe we have to merge File and Files event to single, and use only Files, they basically the same. But can stay as is)

yes that sounds ok only the subject/eventname should differ? So you have a chance to detect which actually triggers it. Maybe different eventnames (file and files) with the same object, in both cases we use subject as array of files?

[Fedik]

only the subject/eventname should differ? So you have a chance to detect which actually triggers it

For this purpose then 2 event is good, maybe can extend one from another, to share validation method.
FilesEvent extends FileEvent or vice versa, what you like more :)

[HLeithner]

The name of the event is set in the constructor which sounds a bit useless when we always have one object per event right?

So changing

$event = new FetchMediaFileEvent('onFetchMediaFile', ['file' => $file]);
to
$event = new FetchMediaFilesEvent('onFetchMediaFile', ['files' => [$file]]);

should fit right?

edit: and of course the following getArgument line... here could be a problem if more then one file is returned...

[Fedik]

yea, kind of it

and of course the following getArgument line... here could be a problem if more then one file is returned

$file = reset($event['files']) only pick first

[Fedik]

Well, then maybe better leave as is

[laoneo]

For me this is the wrong place to add such deep validation for the state where Joomla is now. This must be for now in the model, similar to what we do with forms when saving data.

If you really want to do this, then you need to do it properly, otherwise it looks like a half baked solution when it is implemented in one event that way and in another in a different way. Make it for Joomla 4.1 or 4.2, but then properly across the whole code base. The same what we did with the namespacing of MVC. First we changed com_content and afterwards, when we agreed, then we changed the other extensions. I suggest to do this, otherwise we are going to introduce a new direction in a pr for a minor release, where no developer will figure out what the heck we did here.

[HLeithner]

Sadly the new event system documentation (or plans) aren't really good, at least I didn't found much. But what I know is that it was introduced 2013 so I think it's not new only wrongly implemented for the new events, because it's really much work to do things right...

I wouldn't like to introduce more "half-bake" events. The alternative is to create the interface, class and a trait for the file validation and integrate it the model and the plugin, so it validates if someone sets new values to the new file class. then the plugin only needs to validate all the checks I did for the properties.

[laoneo]

There are multiple ways how to validate the data. An alternative to the interface/class/trait approach would be that the provider manager always returns a wrapper adapter which contains internally the original adapter from the plugin. All function calls are then delegated to the internal adapter and the return value will be validated by the adaptermanager where the wrapper has a reference to. This adapter is also available in the model, so validation can be done then after the events again.

I see it that way, that the model is responsible to deliver data correctly to the controller. Honestly a plugin hasn't to deliver the data perfect as IMO the plugins can be rather dumb. Internally we need to make sure that the data is delivered properly to the front end.

And last but not least, an extension developer has not the intention to deliver incorrect data, as the system would crash then immediately. From my experience it is even the opposite, you have to harden the plugins more than core does as you have to deal with any kind of situation, especially in the plugin system where you have to deal with all kinds of plugins, states, versions and requests.

[Fedik]

Honestly a plugin hasn't to deliver the data perfect as IMO the plugins can be rather dumb.

That the point of "in event validation", it should crash and burn immediately. Not waiting when event call ends and then crash , because then it hard to determine which plugin make an error. That is a main issue of all systems which provide hooks.

an extension developer has not the intention to deliver incorrect data, as the system would crash then immediately

Sure, the developer has not such intention. However, it can happen accidentally (or because lack of knowledge), and be hidden until certain conditions happens.

[laoneo]

Just to be clear here. I'm not against validation. But not in a pr here in a fork of the CMS where developers have to dig deep to find the reason why we did it that way.
ll I'm saying is that make it in it's own pr on the CMS repo with clear explanation and not in a side pr which has a totally different target. Did you understand that?

[laoneo]

Ok, to move here forward. @Fedik are you ok with the changes from @HLeithner ? If yes I will merge it and then we can go ahead with the one in upstream.

[Fedik]

yes, I think it is good for what we have