I have tested this item ✅ successfully on e5dc8dd

I have tested this item ✅ successfully on 40c3b81

I have tested this item ✅ successfully on 40c3b81

@laoneo I have thought about that thing with result for $newUrl, and I think correct would be to get result from the event instead of dispatcher. I suggest you next:

The event class, that have defined URL get/set:

namespace Joomla\Component\Media\Administrator\Event;

class MediaFileUrlEvent extends AbstractEvent
{
    public function __construct($name, array $arguments = array())
    {
        parent::__construct($name, $arguments);

        // Check for required arguments
        if (empty($arguments['url']) || !is_string($arguments['url']))
        {
            throw new BadMethodCallException("Argument 'url' of event $name is empty or not of the expected type");
        }
    }
    public function getUrl(): string
    {
        return parent::getArgument('url');
    }

    public function setUrl(string $url)
    {
        parent::setArgument('url');
    }
}

Triggering the Event:

$url   = $this->getAdapter($adapter)->getUrl($path);
$event = new MediaFileUrlEvent('onFetchMediaFileUrl', ['url' => $url]);

Factory::getApplication()->getDispatcher()->dispatch(
  $event->getName(),
  $event
);

$newUrl = $event->getUrl();

Plugin listener:

public function onFetchMediaFileUrl(MediaFileUrlEvent $event)
{
    $event->setUrl('blabla/my/url.jpg');
}

As you see it more straightforward ;)

@nibra @HLeithner please confirm, or maybe I wrong

Regarding 'get result from the event instead of dispatcher':
This is more a matter of taste. One big source of confusion is that in Joomla, calling a plugin is always called triggering an event, although plugins can be event handlers but also just hooks.

Events are usually just emitted without expecting (or waiting for) results. Other parts of the software or even other systems may or may not react on those events.

Hooks are a technique to inject code - they get some input data and have a return value. Thus they can alter values or provide new data.

In Joomla, we have no distinction between those two approaches. We just have plugins that might or might not return values. In Joomla, we're using events to communicate with the plugins. If a plugin returns a value, it is expected that the value is returned by the dispatch() method.

In the present case, both @laneo's and @Fedik's approaches are valid an can be considered 'clean', while @laoneo's is more 'Joomla like'.

@nibra thanks for clarification

So boys, I hope now everybody is happy and we can get it merged.

@laoneo I made PR to your branch,
then it should be good

Merged

Event calls are now the same across all methods. Can be tested now again.

The event set functions need validators, Like is_array at least for setFiles and is_string for setUrls like in the constructor. Also the parameters must be Immutable (which is already done in the events).

There are no set functions in the new events.

There are no set functions in the new events.

in this case it makes no sense to use return $event->getFiles(); when it's not planned that t he values can be changed by a plugin.

Re reading your opening post you say

During all events the objects can be manipulated, except the getUrl one. There it is possible for subscribers to provide a new url.

doesn't that mean the plugins should/can change the event attributes?

You can change by setArgument() which is a public function in the event. It is intended to give the plugin full control over the returned data. Better would be to make a proposal, instead of getting into an argument. All I want is to have these events in MM without over engineering the whole thing.

@laoneo I think @HLeithner means something like there:

It does not have setter for subject, however the Event class will call protected setSubject, to validate the subject if someone will do $event->setArgument('subject', $foobar);

So we have validation in both cases, in __constructor and setArgument

You can change by setArgument() which is a public function in the event. It is intended to give the plugin full control over the returned data. Better would be to make a proposal, instead of getting into an argument. All I want is to have these events in MM without over engineering the whole thing.

It's not about over engineering it's that the new event system should be clean and safe, that's the reason for validation.

Fedir already wrote it.

Adding

    public function setUrl(string $url)
    {
        if (empty(url))
        {
            throw new BadMethodCallException("Argument 'url' is empty");
        }
        parent::setArgument('url');
    }

would fit the needs (at least for the one plugin).

@HLeithner there a caveat,

public function setUrl(string $url)
{
  if (empty(url))
  {
    throw new BadMethodCallException("Argument 'url' is empty");
  }
  parent::setArgument('url');
}

This will lead to infinity loop ;)
The method should be protected.

@HLeithner there a caveat,

public function setUrl(string $url)
{
  if (empty(url))
  {
    throw new BadMethodCallException("Argument 'url' is empty");
  }
  parent::setArgument('url');
}

This will lead to infinity loop ;)
The method should be protected.

haha I copied you example and you are right should be more like

protected function setUrl(string $url)
{
  if (empty(url))
  {
    throw new BadMethodCallException("Argument 'url' is empty");
  }
  return $url;
}

yeah, originally I also thought it should work,
but after couple tests I ended up with infinity loop :)

So instead of using the events arguments feature you want to store the url in an extra variable in the event class? I mean if a plugin wants to set an empty url then this is fine. If a plugin wants to set empty files this is also fine.

So instead of using the events arguments feature you want to store the url in an extra variable in the event class? I mean if a plugin wants to set an empty url then this is fine. If a plugin wants to set empty files this is also fine.

It's not an instead, that's how the event system works.

If it's ok that the file(s) is empty then we only have to validate that it is a string or an array (depending of the event).

The event constructor uses the the setArgument() function to save the variables in the $this->arguments array of the the event class.

the setArgument function checks if a set() function exists and uses it for validation. if it fails the event can't be created. That's the reason to do extra checks like !empty() in the constructor. In the setUrl() function we would then only validate that the provided parameter is an string.

Also make a pr against my branch with your suggestion.

So changed it to an internal variable, hope @HLeithner is happy now and we can go ahead.

So changed it to an internal variable, hope @HLeithner is happy now and we can go ahead.

it has nothing to do to make me happy, it's about having a "bullet proof" api, which is the reason for the new event system. I will create pull request against your branch or commit directly what ever you prefer.

pr please

created a pull request Digital-Peak#17 would be good if @Fedik can have a look too.

Merged it. @wilsonge all ok here now

@HLeithner can you have a look on the system tests as they are failing since your changes got merged. Thanks.

I can replicate the problem locally but don't understand the reason.
It seems that the DELETE XHR request is not send to the server and the vue application is waiting for feedback that will never come because no request has been created. So I think it's unrelated to the PR but happens reliable in drone for this pr...

I can replicate this problem with Joomla 4.0.1, create a folder, click around (optioal), delete it. Sometimes the loading bar doesn't stop and no XHR DELETE request is send. Not sure for the reason. So may be completely unrelated to the current error.

Are you able to fix it?

Are you able to fix it?

wasted after of the day yesterday to debug drone but only found out that it fails as soon as line
$event = new FetchMediaFileEvent('onFetchMediaFile', ['file' => $file]);
is executed.

I debug this on my github account to reduce notifications to others HLeithner#38

last try was successful and I think (while writing this) that the problem is an empty directory.

@laoneo I think I would the reason the file API also sense directories against the File event

  [0]=>
  object(stdClass)#885 (13) {
    ["type"]=>
    string(3) "dir"
    ["name"]=>
    string(11) "test-folder"
    ["path"]=>
    string(34) "local-images:/test-dir/test-folder"
    ["extension"]=>
    string(0) ""
    ["size"]=>
    string(0) ""
    ["mime_type"]=>
    string(9) "directory"
    ["width"]=>
    int(0)
    ["height"]=>
    int(0)
    ["create_date"]=>
    string(25) "2021-11-08T20:09:15+00:00"
    ["create_date_formatted"]=>
    string(16) "2021-11-08 20:09"
    ["modified_date"]=>
    string(25) "2021-11-08T20:09:15+00:00"
    ["modified_date_formatted"]=>
    string(16) "2021-11-08 20:09"
    ["adapter"]=>
    string(12) "local-images"
  }

Which fails the file validation, so the question is now how to fix this. Add an additional event type or make the File event more generic. I would expect that a plugin would handle files and directories different.

There is no reason to handle directories different than files. Sending two events makes no sense as the result will be combined again and you loose the sorting when you combine them afterwards. All a developer wants is to have the possibility to interact with the result set before sending it to the client. I would suggest that your validation changes will be reverted and then when this one gets merged you can open a new pr with the validation stuff, so we can move forward with this pr. Otherwise it will never arrive in core and I need it for several reasons.

I think there is no need to rush this, as we have plenty of time to get it merged till the next release. There are several people supporting this function, so it's very likely that it's getting merged. BUt we should really go for best practise here.

I see two things which has to be done:

• Rename the events (sorry for that) to communicate that there are possible files AND folders (e.g. FetchMediaItemEvent, FetchMediaListEvent, ...? anyone?)
• Add the validation. As @HLeithner did the first validation, probably he will provide the last bits, too?

If both are ready and tested, I'm happy to merge.

* Rename the events (sorry for that) to communicate that there are possible files AND folders (e.g. FetchMediaItemEvent, FetchMediaListEvent, ...? anyone?)

The function names are getFile/getFiles, in Media Manager we always talk about files and not items. Adding now a different therm would confuse the developers.

I agree with @laoneo it does not make sense for 2 events.
Model::getFile[s]() handle both folder and files.

hm, only validation for type is

okay, I see, it need to exclude extension validation if type is DIR (or just remove this section).
Next will fail for folders always:

A File is not a Folder, so a plugin developer would expect only a file, I don't know who named the function getFile but it seems it's simple wrong. Continuing the wrong wording make it worse.

Since we have now the time to do it right and don't need to rush out anything we should not introduce new "wrong" names.

Allon and fedir is right, having separate events is not needed since the type is in the object. Solving the validtion should be trivial (I will do this). I would also rename (in a b/c way, even if we have no b/c promise on this) the getFile(s) function to getItem(s) to reflect the real behavior.

But a folder is a file.

But a folder is a file.

yes in posix land because everything is a file... but we have separate functions to createFile and createFolder seems not to be the same.

Calm down, I will rename the events. All I want are the events, if the developers will understand then better, I'm fine with this.

System test still fails

System test still fails

Has also to be fixed in the Files variant of the Event.

Can you do that please.

Still not

You can push directly to the branch if you want. No need to make a suggestion first.

It also need to skip: size, width, height and other non folder thing.
btw, width, height valid only for images, and invalid for pdf and other documents and audio files

It also need to skip: size, width, height and other non folder thing. btw, width, height valid only for images, and invalid for pdf and other documents and audio files

if I'm not wrong I only check if it's an integer not if is > 0. What I have check in the original class it sets the values to 0.

if (!isset($value->width) || !is_integer($value->width))

isset() will return false if $value->width === null or not exists,
and I guess for a folder it can be one or another

Events have been renamed. @HLeithner can you please fix your part please.

@HLeithner can you please have a look on the errors?

I have tested this item ✅ successfully on c42a582

All seems works now, @HLeithner please confirm when you will have a min.

thanks @laoneo for fixing it your self and simplify the validation. thanks @Fedik for testing.
It looks ok for me. Maintainers have to decide in which branch it should be merged @bembelimen

I have tested this item ✅ successfully on c42a582

RTC

I'm pretty sure, this should go into 4.1.

Initially the changes have been small so it made sense at that time to go into 4.0. Now it makes more sense for 4.1. Rebased.

If you allow maintainer to change your code, I could trigger the sync with the base branch and merge. So for you you have to do it @laoneo , could you please do it?

@bembelimen , the maintainer checkbox works only for personal repos
https://github.community/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847/5

Ah ok, thx.
Would you mind updating it so I can merge?

Did already

Thx