NPM Resource Changed ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
10 May 2021

Pull Request for Issue #26750.

Summary of Changes

This is my quick attempt to solve the issue with external adapters discussed in #26750. The change includes:

  • For local adapter, it will just work as how it was before
  • For external adapters:
  • Store full url of the image into value of the field so that we do not have to ask the adapter to get the image url each time the image is being displayed
  • The adapter + the internal path of the image is stored as part of value, too. It is added to the end of the image URL (# + Adapter Name + Internal Path). I decided to use # character instead of & character because Dropbox does not allow adding query string to image url.
  • Modify HTMLHelper to remove the adater name + Internal Path from the image before it is displayed

Testing Instructions

  1. Download update package for this PR at https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/33724/downloads/44325/Joomla_4.0.0-beta8-dev+pr.33724-Development-Update_Package.zip . Go to System -> Update -> Joomla, upload and update this package.
  2. General testing is test and make sure you can select image for your articles and these selected images will still be displayed properly when you view the article from frontend of your site.

Test local adapter:

  • Create a article, select an image for that article, make sure you can still select the image. Save the article .
  • Access to the article from frontend, make sure the image is being displayed
  • Edit that article again. Click on the Select button next to the image and try to select new image. Please make sure that you don't see any error while clicking on that Select button. Also, click on that Select button should show you the folder which the image belong to (basically, the system remember the path of the image, and you do not have to navigate from root to find a new image.

Test External Adapter

It will be good to test media manager with an external adapter like dropbox and make sure it is working OK. You can"

Client ID : 2p8aoyqpeub1a39
Client Secret: 2p8aoyqpeub1a39
Access Token: jeU8dH-JufAAAAAAAAAAAex3adAkjxBV5e7BqzrOi6T60ZRzwhVQUNJDOnmitGOa

Plugins-DPDropbox-Joomla-4-Administration

Then try to create article or edit article, select an image from dropbox adapter for that article, save the article. Then view that article from frontend, make sure the image is still being displayed.

avatar joomdonation joomdonation - open - 10 May 2021
avatar joomdonation joomdonation - change - 10 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 May 2021
Category JavaScript Repository NPM Change Layout Libraries
avatar joomdonation joomdonation - change - 10 May 2021
Title
Support External Adapters For Media Form Field
[4.0] Support External Adapters For Media Form Field
avatar joomdonation joomdonation - edited - 10 May 2021
avatar joomdonation
joomdonation - comment - 10 May 2021

One thing I should mention here is that for image from external adapter, the system remember the path of the image. Next time when you edit the image, it will remember the folder of that image, so you won't have to navigate from root folder. Maybe we should try to do the same for local adapter.

avatar HLeithner
HLeithner - comment - 10 May 2021

Don't know where to start ;-)

if we are going to use # then we should also change the jooma_image_width to use this. Also please don't do the parsing manually. It should be possible to use uri to parse the image meta information string somthing like this:

$imageUrl = 'images/bla.jpg#joomlaImage://local-0?width=123&height=321&path=/images';

$url = new Uri($image);

$metadata = new Uri($url->getFragment());

local-0:// should be local-name://

joomla_image_width -> width
joomla_image_height -> height

of course it would be possible to explode $imageUrl at #

avatar dgrammatiko
dgrammatiko - comment - 10 May 2021

joomla_image_width -> width
joomla_image_height -> height

B/C break, just saying

avatar HLeithner
HLeithner - comment - 10 May 2021

joomla_image_width -> width
joomla_image_height -> height

B/C break, just saying

Depends how you are doing it ;-) if you already parse the $imageUrl with JUri you have this parameters and can use them too

avatar joomdonation joomdonation - change - 10 May 2021
Labels Added: NPM Resource Changed ?
avatar joomdonation
joomdonation - comment - 10 May 2021

Thinking more about it, I wonder if this solution will work. If the real link of the external image is expired for some reason, it won't be displayed anymore (so for Dropbox for example, I don't know if the link is there for every or it will be expired at certain time). If it is expired, then this solution won't work.

avatar richard67
richard67 - comment - 10 May 2021

Anyway the scss linter complains again, this time about unused variables: https://ci.joomla.org/joomla/joomla-cms/43375/1/21 .

avatar joomdonation
joomdonation - comment - 10 May 2021

Thanks @richard67 Please ignore code style issue for now. I just want to see if the PR works or not first before working on further cleanup

avatar richard67
richard67 - comment - 10 May 2021

@joomdonation Code style errors make the system test not being run, so they should be avoided even in working phase because system tests can show errors.

avatar richard67
richard67 - comment - 10 May 2021

Code style is fixed now. Currently analysis4x fails in Drone, very likely due to false alarm. Will this PR receive further changes? If so, we should wait with fixing the analysis4x thing. But if the PR is ready let me know, then I will ping the experts for fixing it.

avatar joomdonation
joomdonation - comment - 10 May 2021

In the last commit, I implemented the new change suggested by @HLeithner . The value for the image will now has this sample format images/headers/walden-pond.jpg#joomlaImage://local-0/headers/walden-pond.jpg?width=700&height=180

I tested it myself and it works OK. I think it implemented the changes which we discussed so far OK now

Please note that this PR will only work if the external adapter can implement a method return permanent URL of the image. The Dropbox adapter which I am using for testing can only get temporary link (which will be expired in 4 hours), so it is not really working. I looked at Dropbox API and seems we can get permanent link, so in theory, this PR should work.

avatar joomdonation
joomdonation - comment - 10 May 2021

I'm unsure why drone failed for this PR https://ci.joomla.org/joomla/joomla-cms/43404/1/27

avatar rdeutz
rdeutz - comment - 10 May 2021

@zero-24 @SniperSister can you have a look at rips, thanks

avatar Fedik
Fedik - comment - 10 May 2021

Thanks for the PR.

One thing I should mention here is that for image from external adapter, the system remember the path of the image. Next time when you edit the image, it will remember the folder of that image, so you won't have to navigate from root folder.

I have wrote about it also #26750 (comment)
In theory, if we store the source URI correctly then we can write some script that check/validate paths.

The value for the image will now has this sample format images/headers/walden-pond.jpg#joomlaImage://local-0/headers/walden-pond.jpg?width=700&height=180

To me @HLeithner path #33724 (comment) a bit to much ;)
I would try to keep the original URI as it is local-0:/headers/walden-pond.jpg

Maybe more simple:
images/headers/walden-pond.jpg#local-0:/headers/walden-pond.jpg?width=700&height=180
or
images/headers/walden-pond.jpg?joomla_file_uri=local-0:/headers/walden-pond.jpg&width=700&height=180

Unfortunately local-0: is not really a schema, but some fancy prefix :)

Check how it parsed in the media controller:

private function getAdapter()
{
$parts = explode(':', $this->input->getString('path', ''), 2);
if (count($parts) < 1)
{
return null;
}
return $parts[0];
}
/**
* Get the Path.
*
* @return string
*
* @since 4.0.0
*/
private function getPath()
{
$parts = explode(':', $this->input->getString('path', ''), 2);
if (count($parts) < 2)
{
return null;
}
return $parts[1];
}

Or what do you think @HLeithner ?

avatar joomdonation
joomdonation - comment - 10 May 2021

@Fedik The format suggested from @HLeithner is joomlaImage://local-0/headers/walden-pond.jpg?width=700&height=180 (of course, it is just a sample)

The purpose is that it allows us to use our Uri class to parser the data and get the correct information we want. So local-0 is host, headers/walden-pond.jpg is path, width=700&height=180 is query string. It will allow us to parse and get the data easier.

avatar Fedik
Fedik - comment - 10 May 2021

So local-0 is host,

I thought the same before, but the problem that it not really a host ;)
Look how media manager parse it in php backend.

Sorry I mixed host and schema

avatar joomdonation
joomdonation - comment - 10 May 2021

Look how media manager parse it in php backend

I haven't looked at it yet. But I guess it is some kind of string manipulation?

avatar Fedik
Fedik - comment - 10 May 2021

But I guess it is some kind of string manipulation?

yeah, just explode(':', $path, 2), look the link in my previous comment

avatar Fedik
Fedik - comment - 10 May 2021

okay, maybe it can be as @HLeithner suggested

avatar joomdonation
joomdonation - comment - 10 May 2021

Actually, I'm find with any format :). We just need to make decision about the format we want to use :)

avatar zero-24
zero-24 - comment - 10 May 2021

@zero-24 @SniperSister can you have a look at rips, thanks

We are looking into it and report back here please do not merge this as long as rips fails.

avatar HLeithner
HLeithner - comment - 10 May 2021

the schema follows more your suggestion @Fedik it would allow us (if needed at some point) to create a php stream wrapper.

And it's a standard format which we have a parse in every language ;-) That's the reason I suggested the change to this.

images/headers/walden-pond.jpg#local-0:/headers/walden-pond.jpg?width=700&height=180

this can't be parse by JURI because we don't have a ://

images/headers/walden-pond.jpg?joomla_file_uri=local-0:/headers/walden-pond.jpg&width=700&height=180

This breaks the image if some may not filter the url right for what ever reason. It's easier to explode on # because then on ? because you may have get parameters for the image...

Think about this I wonder if removing the # fragment we may break svg images? Is this possible?

avatar dgrammatiko
dgrammatiko - comment - 10 May 2021

Think about this I wonder if removing the # fragment we may break svg images? Is this possible?

Valid

Edit: Actually this is invalid as the media manager is not having any means to return a specific node of an SVG element, we can only return only the SVG file.

avatar HLeithner
HLeithner - comment - 10 May 2021

so using only the last # would be better and validate the schema

avatar Fedik
Fedik - comment - 10 May 2021

all good then

it would allow us (if needed at some point) to create a php stream wrapper.

stream wrapper would be much nice,
some time in future :)

avatar HLeithner
HLeithner - comment - 10 May 2021

all good then

it would allow us (if needed at some point) to create a php stream wrapper.

stream wrapper would be much nice,
some time in future :)

maybe it would make sense to check the mediamanager code if change the complete schema todo format? don't know how much it's used inside mediamanager...

avatar joomdonation
joomdonation - comment - 11 May 2021

So I take a look at our MediaField code https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Field/MediaField.php#L253-L267 and see that currently, we always check local directory. That mean for a Media field type, it is not possible to use a folder/relative path for external adapter. Any idea how we should handle that?

Maybe allow syntax such as directory="dpdropbox-0:/photos" ?

avatar HLeithner
HLeithner - comment - 11 May 2021

So I take a look at our MediaField code https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Field/MediaField.php#L253-L267 and see that currently, we always check local directory. That mean for a Media field type, it is not possible to use a folder/relative path for external adapter. Any idea how we should handle that?

Maybe allow syntax such as directory="dpdropbox-0:/photos" ?

Code looks wrong :(

Not sure what joomla-media-field needs, if it require the adapter and the path separated then it would make sense to provide it in 2 attributes. In case it only pass it to the mediamanger we can keep only a single string

avatar joomdonation
joomdonation - comment - 11 May 2021

Not sure what joomla-media-field needs, if it require the adapter and the path separated then it would make sense to provide it in 2 attributes. In case it only pass it to the mediamanger we can keep only a single string

I'm unsure, too. Maybe we should support a new attribute adapter and keep directory attribute, there. adapter should default to local for backward compatible purpose. @dgrammatiko Do you have any idea about this?

avatar Fedik
Fedik - comment - 11 May 2021

maybe it would make sense to check the mediamanager code if change the complete schema todo format?

Most of the code in ApiController, and ApiModel, as far as I can see.
Simplified version is:

list($adapter, $sourcePath) = explode(':', $path, 2);
$apiModel->getAdapter($adapter)->doStuff($sourcePath); // ->copy(), ->move(), etc

Someone tried to implement kind of stream wrapper in own way :)
It looks limited to use with com_media.

Maybe allow syntax such as directory="dpdropbox-0:/photos" ?
Code looks wrong :(

It is what media manager expects, however media field do not know it.
In theory can add one more IF with if(strpos($this->value, ':'))
before if ($this->value && is_file(JPATH_ROOT . '/' . $this->value))

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

My 2c here, I would add an adapter field in the XML, type string and in the logic I would change the else part from $mediaPath = 'local-0:/' . $folder; to something like:

if ($adapter) {
  // The adapter was set either in the XML or programmatically
  $mediaPath = $adapter . ':/' . $folder;
} else {
 // No adapter is set so we will get the default from com_media
 Boot('com_media')->getDefaultAdapter(); // This is pseudocode...
}
avatar HLeithner
HLeithner - comment - 11 May 2021

My 2c here, I would add an adapter field in the XML, type string and in the logic I would change the else part from $mediaPath = 'local-0:/' . $folder; to something like:

if ($adapter) {
  // The adapter was set either in the XML or programmatically
  $mediaPath = $adapter . ':/' . $folder;
} else {
 // No adapter is set so we will get the default from com_media
 Boot('com_media')->getDefaultAdapter(); // This is pseudocode...
}

in which XML do you mean?

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

in which XML do you mean?

The one that defines the field in a form, eg as in the original issue:

<field
name="imageurl"
type="media"
label="COM_BANNERS_FIELD_IMAGE_LABEL"
directory="banners"
hide_none="1"
size="40"
/>

avatar HLeithner
HLeithner - comment - 11 May 2021

no idea why there is a directory attribute in the form field, maybe as default start value?
The adapter (and the path to the image) can never be stored in the XML because it can be changed if while selecting the image. That's the point why I want to have it as parameter attached to the image url

avatar joomdonation
joomdonation - comment - 11 May 2021

It's right that directory is the starting point of which media manager loaded when you click on the file. The adapter could be a new attribute to the field, but it should default to local adapter for backward compatible purpose.

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

no idea why there is a directory attribute in the form field, maybe as default start value?

It serves a purpose: to open the media manager in the correct folder.

My idea is to have in the options on the com_banners a field that sets the adapter and another for the initial path. Then programmatically (this is the important part here) pass these values to the field. Hard coding these values in a dynamic and flexible system will never work (eg delete all the adapters and the install let's say cloudinary as the only adapter and the system will just fail badly)

avatar HLeithner
HLeithner - comment - 11 May 2021

It's right that directory is the starting point of which media manager loaded when you click on the file. The adapter could be a new attribute to the field, but it should default to local adapter for backward compatible purpose.

Ok the directory is only use full if you didn't select and image and if you use the local adapter (a default that supports directory structure), if you don't have the local adapter the directory is likely useless. Not really a good solution in my opinion but ok.

no idea why there is a directory attribute in the form field, maybe as default start value?

It serves a purpose: to open the media manager in the correct folder.

My idea is to have in the options on the com_banners a field that sets the adapter and another for the initial path. Then programmatically (this is the important part here) pass these values to the field. Hard coding these values in a dynamic and flexible system will never work (eg delete all the adapters and the install let's say cloudinary as the only adapter and the system will just fail badly)

Actually that's not true, if the you open the mediamanager with 'joomlaImage://dropboxdp/banner/image4711.jpg' and the banner directory doesn't exists the mediamanager starts in the root of dropboxdp, if the adapter dropboxdp doesn't exists it will use the 'default' adapter (we really need a solution for the default adapter). No reason to fail just fall back to the default.

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

No reason to fail just fall back to the default.

Yes, fallbacks are fine but it's not ONLY for the com_media's default adapter, it's also about setting a preferred adapter per component/form...

avatar HLeithner
HLeithner - comment - 11 May 2021

No reason to fail just fall back to the default.

Yes, fallbacks are fine but it's not ONLY for the com_media's default adapter, it's also about setting a preferred adapter per component/form...

This couldn't be static too but isn't the main problem at the moment. If we have a getDefaultAdapter() function it can be extended by context like com_contact.contact for example. But defining the adapter in the xml doesn't makes any sense, also the directory could/should be context based.

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

But defining the adapter in the xml doesn't makes any sense

Oh, I think you misunderstood me, if there is a way to expose the property adapter of the field without having an attribute then do that. For me would be way easier to handle things programmatically by setting the attribute but that's just me...

avatar Fedik
Fedik - comment - 11 May 2021

Boot('com_media')->getDefaultAdapter(); // This is pseudocode...

If we have a getDefaultAdapter() function it can be extended by context like com_contact.contact for example.

I think I have wrote it before, it maybe better if it comes from com_media parameters.

The "context" does not need, it should be kind of global for com_media. Example: User choose to store all files in dropbox by default, etc.

But defining the adapter in the xml doesn't makes any sense, also the directory could/should be context based.

That right, does not need atribute.
The directory will depend from "default adapter" (from com_media parameters)

avatar HLeithner
HLeithner - comment - 11 May 2021

But defining the adapter in the xml doesn't makes any sense

Oh, I think you misunderstood me, if there is a way to expose the property adapter of the field without having an attribute then do that. For me would be way easier to handle things programmatically by setting the attribute but that's just me...

^^ hopefully we now talk about the same ;-)

Boot('com_media')->getDefaultAdapter(); // This is pseudocode...

If we have a getDefaultAdapter() function it can be extended by context like com_contact.contact for example.

I think I have wrote it before, it maybe better if it comes from com_media parameters.

The "context" does not need, it should be kind of global for com_media. Example: User choose to store all files in dropbox by default, etc.

The feature would be that the user can select which component has which starting directory, that's nothing for 4.0. But could be usefull if all your com_contact images are on google and all com_banner images are on dropbox. and Com_content images are provided by nextcloud.

But yes the first step would be to make it configurable in com_media configuration.

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

User choose to store all files in dropbox by default, etc.

So EVERY components/form should use JUST ONE adapter. That's doesn't sound quite flexible to me. Just give me a way to pass the adapter either through an XML attribute or another way BUT PLEASE don't choose for me...

avatar joomdonation
joomdonation - comment - 11 May 2021

I think I agree with @dgrammatiko . For my own component for example, I want to get images from a specific folder which is created during installation. For that, I would want to use local adapter, and set directory to the folder I want. I don't think I want to depend on the adapter from media component configuration.

It's not hurt to introduce support for adapter attribute in the xml field definition. The default value for adapter, if not set, could be get from default adapter config option of media component.

avatar joomdonation
joomdonation - comment - 11 May 2021

Or if not, maybe improve the current code so that someone can set directory to use the adapter they want. So directory="local-0:/images/banners".......

avatar HLeithner
HLeithner - comment - 11 May 2021

User choose to store all files in dropbox by default, etc.

So EVERY components/form should use JUST ONE adapter. That's doesn't sound quite flexible to me. Just give me a way to pass the adapter either through an XML attribute or another way BUT PLEASE don't choose for me...

I don't choose for you but you shouldn't choose for the user ;-) in the end the use should have the possibility to select the default adapter/path for your component. That can't be in the xml because you may don't have a local-0 and the user want's another adapter or default directory then you. But that's something not for now. For this the joomlaImage://adapter/path?width= format handle this. So your component can set the default in one way maybe by xml attribute but that can't be changed by the user.

I think I agree with @dgrammatiko . For my own component for example, I want to get images from a specific folder which is created during installation. For that, I would want to use local adapter, and set directory to the folder I want. I don't think I want to depend on the adapter from media component configuration.

It's not hurt to introduce support for adapter attribute in the xml field definition. The default value for adapter, if not set, could be get from default adapter config option of media component.

I think a component can't provide a useful default adapter except it provides it's own adapter, in all other cases it would be local-0 which should be changed to something like local-images or local-default

avatar joomdonation
joomdonation - comment - 11 May 2021

I think a component can't provide a useful default adapter except it provides it's own adapter, in all other cases it would be local-0 which should be changed to something like local-images or local-default

We support directory attribute for the field, so without adapter support, the directory won't be reliable. For example, in my own component, I set directory="com_mycom". The purpose is when users click on the button to select an image for the item, images/com_mycom will be starting folder. Image that someone selects Dropbox as default adapter from media component, this won't work because the folder does not exist.

avatar HLeithner
HLeithner - comment - 11 May 2021

I think a component can't provide a useful default adapter except it provides it's own adapter, in all other cases it would be local-0 which should be changed to something like local-images or local-default

We support directory attribute for the field, so without adapter support, the directory won't be reliable. For example, in my own component, I set directory="com_mycom". The purpose is when users click on the button to select an image for the item, images/com_mycom will be starting folder. Image that someone selects Dropbox as default adapter from media component, this won't work because the folder does not exist.

it would only work if you have an adapter local-0 but you component doesn't know (at development time) if there is an adapter local-0. So I mean you can add it but it's useless if in joomla 4.1 we change the default adapter from local-0 to local-default.

avatar joomdonation
joomdonation - comment - 11 May 2021

Hmm. Long discussion and maybe could not come to end :D. So with the current state, could you let me know what change is needed so that I can find sometime to finish this PR?

avatar HLeithner
HLeithner - comment - 11 May 2021

Hmm. Long discussion and maybe could not come to end :D. So with the current state, could you let me know what change is needed so that I can find sometime to finish this PR?

Sadly I lost track of the PR it self... I check it later again.

avatar joomdonation
joomdonation - comment - 11 May 2021

In case someone wants to try, you can install this dropbox adapter and use my test dropbox account for seeing how it works (note that the current dropbox adapter can only get temp url for the image, so the url will be invalid after 4 hours)

plg_filesystem_dpdropbox-master.zip

Client ID : 2p8aoyqpeub1a39
Client Secret: 2p8aoyqpeub1a39
Access Token: jeU8dH-JufAAAAAAAAAAAex3adAkjxBV5e7BqzrOi6T60ZRzwhVQUNJDOnmitGOa

avatar dgrammatiko
dgrammatiko - comment - 11 May 2021

That can't be in the xml because you may don't have a local-0 and the user want's another adapter or default directory then you. But that's something not for now

I think the problem here is that misinterpret what I proposed. I said DEFINE an XML attribute for the field but I didn't say prepopulate it or hardcode a value. Is there another way to pass properties to the field apart from the defined ones (as XML attributes)?

avatar joomdonation joomdonation - change - 14 May 2021
Labels Removed: NPM Resource Changed
avatar joomdonation joomdonation - change - 14 May 2021
Labels Added: NPM Resource Changed
avatar joomdonation
joomdonation - comment - 14 May 2021

@dgrammatiko With last commit, you can set set the adapter you want to use for your media field via directory attribute of the field. For example:

directory="dpdropbox-0:/photos"

Or

directory="local-0:/sampledata/categories"

I think it should address your concern and make the field flexible enough for usage.

There are still some limitation with the current code which I do not see an easy way to address:

  • For image value stored in old format (for example, uploaded before this change or migrated from Joomla 3), we have to hardcode adapter to local-0
  • For field (XML) which as directory attribute set to certain value and that value doesn't contain adapter information, we have to hardcode adapter to local-0, too

I think that's necessary for backward compatible purpose and maybe it's OK to accept this limitation. Other than that, with the current state, the PR is fine, I think.

avatar joomdonation
joomdonation - comment - 14 May 2021

@HLeithner Could you please have a discussion with @bembelimen to see if the approach which we are using on this PR is OK?

avatar joomdonation joomdonation - change - 14 May 2021
Labels Added: ?
avatar joomdonation
joomdonation - comment - 14 May 2021

Drone/RIPS is still failed for this PR for unknow reasons :(.

avatar Quy Quy - change - 14 May 2021
Labels Added: ?
Removed: ?
4f40530 14 May 2021 avatar Quy Typo
avatar joomdonation joomdonation - change - 14 May 2021
The description was changed
avatar joomdonation joomdonation - edited - 14 May 2021
avatar joomdonation joomdonation - change - 14 May 2021
Labels Added: ?
Removed: ?
avatar joomdonation
joomdonation - comment - 15 May 2021

@zero-24 @SniperSister Could you please help looking to see why RIPS is fail here? Would love to get this release blocker PR finished.

avatar rdeutz
rdeutz - comment - 21 May 2021

@joomdonation the Rips problem is sovled

avatar joomdonation
joomdonation - comment - 21 May 2021

Thanks @rdeutz. Need some more clean up to make it ready to merge. Will wait for @dgrammatiko PR #34069 ready and I will finish this one. If it goes well, we can have it finished during this weekend.

avatar joomdonation joomdonation - change - 22 May 2021
Labels Added: ?
Removed: ?
avatar dgrammatiko
dgrammatiko - comment - 22 May 2021

images/headers/walden-pond.jpg?joomla_file_uri=local-0:/headers/walden-pond.jpg&width=700&height=180

I'm a little bit worried about the plain width and height URL params. Are we sure that these will not clash with some external adapter's returned path?

avatar joomdonation
joomdonation - comment - 22 May 2021

@dgrammatiko Exactly what's the problem you are worry about? For external adapter, width and height might not be available and in this case, width and height will be removed by cleanImageURL method. Will that be OK ?

I don't know where we use that data for, so I'm unsure if that would work. But it should be the same with how it works before for local adapter.

avatar dgrammatiko
dgrammatiko - comment - 22 May 2021

I don't know where we use that data for,

This is for the loading=lazy tag on the images. The possible problem here is that we append URL params in the local-0:/headers/walden-pond.jpg which if it already has these params it will result to double entries. I have no idea if there are services out there that use directly height and width as URL params but if there are this would be problematic

avatar brianteeman
brianteeman - comment - 22 May 2021

Dont kn ow if it helps but cloudinary uses dynamic urls like this
https://res.cloudinary.com/demo/image/upload/w_170,h_153,c_scale/turtles.jpg

w_170 = width 170px
h_153 = height 153px

avatar dgrammatiko
dgrammatiko - comment - 22 May 2021

Dont kn ow if it helps but cloudinary uses dynamic urls like this
https://res.cloudinary.com/demo/image/upload/w_170,h_153,c_scale/turtles.jpg

So my concern is that if there's a service out there with a returning URL like https://res.cloudinary.com/demo/image/upload/turtles.jpg?with=100&height=100 there will be a clash. Cloudinary is not using URL params but we can't rule out that these params are unique. That was the reason I had prefixed them with joomla_image_ (probably overkill too many characters...) but maybe I'm just overcautious here

avatar dgrammatiko
dgrammatiko - comment - 22 May 2021

Also the dropbox plugin seems broken?
Screenshot 2021-05-22 at 14 02 33

avatar joomdonation joomdonation - change - 22 May 2021
Labels Added: ?
Removed: ?
avatar joomdonation
joomdonation - comment - 22 May 2021

@dgrammatiko @brianteeman I don't really understand how we are using with and height to handle lazy load yet, so I could not answer the question about how width and height causes any issue if it is part of the returned image URL

@dgrammatiko In the last commit, I decided to not hardcode the adapter to local-images anymore. The system will try to use the top level folder from the part and validate and make sure it is configured in the adapter before using it in the adapter name. That's too avoid un-friendly error message displayed if someone, folder some reasons, choose to not use images folder in local adapter for their new Joomla installation. This makes the field a bit more flexible.

Attached is the dropbox plugin which I'm using on my local installation for testing in case you want to play with it

plug_filesystem_dpdropbox.zip

avatar HLeithner
HLeithner - comment - 22 May 2021

images/headers/walden-pond.jpg?joomla_file_uri=local-0:/headers/walden-pond.jpg&width=700&height=180

I'm a little bit worried about the plain width and height URL params. Are we sure that these will not clash with some external adapter's returned path?

we don't user this uri pattern we use the # pattern so even this is used as image url the with parameter is only a fragment

edf53af 22 May 2021 avatar Quy since
avatar Quy Quy - change - 22 May 2021
Labels Added: ?
Removed: ?
avatar joomdonation joomdonation - change - 22 May 2021
Labels Added: ?
Removed: ?
avatar joomdonation
joomdonation - comment - 22 May 2021

So I think it works OK now (although I don't like much, still many string manipulation). @dgrammatiko @Fedik @HLeithner Do you want to do a code review before we call for end users testing ?

avatar dgrammatiko dgrammatiko - test_item - 22 May 2021 - Tested successfully
avatar dgrammatiko
dgrammatiko - comment - 22 May 2021

I have tested this item successfully on cfa5bd2

I have tested only the local adapter


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

avatar richard67
richard67 - comment - 23 May 2021

@SniperSister @zero-24 The RIPS problem is back here for this PR, it seems.

avatar HLeithner
HLeithner - comment - 24 May 2021

@laoneo can you test this pr with your plugin if it still works as expected?

avatar joomdonation joomdonation - change - 24 May 2021
The description was changed
avatar joomdonation joomdonation - edited - 24 May 2021
avatar joomdonation
joomdonation - comment - 24 May 2021

@HLeithner I updated testing instructions with the modified dropbox plugin. The old plugin from @laoneo is not compatible anymore (we changed from numeric index to adapter name index as discussed - a PR from @dgrammatiko was merged)

avatar richard67
richard67 - comment - 24 May 2021

I've tested the local adapter with success.

But the dropbox thing doesn't work for me.

As soon as the plugin is installed and enabled with the provided access information and I go to the media manager or some media field, I get a PHP error Call to undefined method GuzzleHttp\Utils::chooseHandler().

Call stack:

#  Function                                                                         Location
1  ()                                                                               JROOT/libraries/vendor/guzzlehttp/guzzle/src/functions.php:61
2  GuzzleHttp\choose_handler()                                                      JROOT/plugins/filesystem/dpdropbox/vendor/guzzlehttp/guzzle/src/HandlerStack.php:42
3  GuzzleHttp\HandlerStack::create()                                                JROOT/plugins/filesystem/dpdropbox/vendor/guzzlehttp/guzzle/src/Client.php:65
4  GuzzleHttp\Client->__construct()                                                 JROOT/plugins/filesystem/dpdropbox/vendor/srmklive/flysystem-dropbox-v2/src/Client/DropboxClient.php:105
5  Srmklive\Dropbox\Client\DropboxClient->setClient()                               JROOT/plugins/filesystem/dpdropbox/vendor/srmklive/flysystem-dropbox-v2/src/Client/DropboxClient.php:85
6  Srmklive\Dropbox\Client\DropboxClient->__construct()                             JROOT/plugins/filesystem/dpdropbox/src/Adapter/DPDropboxAdapter.php:96
7  DigitalPeak\Plugin\Filesystem\Dpdropbox\Adapter\DPDropboxAdapter->getClient()    JROOT/plugins/filesystem/dpdropbox/src/Adapter/DPDropboxAdapter.php:80
8  DigitalPeak\Plugin\Filesystem\Dpdropbox\Adapter\DPDropboxAdapter->__construct()  JROOT/plugins/filesystem/dpdropbox/dpdropbox.php:145
9  PlgFileSystemDpdropbox->getAdapters()                                            JROOT/administrator/components/com_media/src/Model/MediaModel.php:52
10 Joomla\Component\Media\Administrator\Model\MediaModel->getProviders()            JROOT/libraries/src/MVC/View/AbstractView.php:146
11 Joomla\CMS\MVC\View\AbstractView->get()                                          JROOT/administrator/components/com_media/src/View/Media/HtmlView.php:64
12 Joomla\Component\Media\Administrator\View\Media\HtmlView->display()              JROOT/libraries/src/MVC/Controller/BaseController.php:692
13 Joomla\CMS\MVC\Controller\BaseController->display()                              JROOT/libraries/src/MVC/Controller/BaseController.php:730
14 Joomla\CMS\MVC\Controller\BaseController->execute()                              JROOT/libraries/src/Dispatcher/ComponentDispatcher.php:146
15 Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch()                            JROOT/libraries/src/Component/ComponentHelper.php:389
16 Joomla\CMS\Component\ComponentHelper::renderComponent()                          JROOT/libraries/src/Application/AdministratorApplication.php:143
17 Joomla\CMS\Application\AdministratorApplication->dispatch()                      JROOT/libraries/src/Application/AdministratorApplication.php:186
18 Joomla\CMS\Application\AdministratorApplication->doExecute()                     JROOT/libraries/src/Application/CMSApplication.php:278
19 Joomla\CMS\Application\CMSApplication->execute()                                 JROOT/administrator/includes/app.php:63
20 require_once()                                                                   JROOT/administrator/index.php:32 

Am testing on Linux.

avatar richard67
richard67 - comment - 24 May 2021

Could be a problem of that plugin. If on Linux only, it's maybe related to file names with wrong case so some class is not found, but that's just a guess. But @joomdonation can't reproduce it on Windows, so maybe not a bad guess?

avatar richard67
richard67 - comment - 24 May 2021

What confuses me is that the plugin ships with an own guzzlehttp but the call stack ends in the one shipped by the CMS.

avatar chmst
chmst - comment - 24 May 2021

I have tested the external adapter on win10, in xampp (error-reporting: maximum).
The plugin is installed without error.
Added the data as given in testing instructions.
Selecting an image from the dropbox works. But the image then has dimension 0/0 and therefor is not visible.

grafik

avatar joomdonation
joomdonation - comment - 24 May 2021

So @chmst tested on Window and it is working well, too. Guess there is a problem with Dropbox adapter on Linux which I don't have time to debug yet. It is of course, not related to this PR

There is a small issue remaining and not related to this PR, too. When insert image from Dropbox using CMS Content -> Image, for external adapter, there is no width and height returned, and in that case, it is invisible in the editor. It is related to how we handle lazy load in the code, so I think I can ask @dgrammatiko to look at it on a separate PR.

avatar joomdonation
joomdonation - comment - 24 May 2021

To be more clear, I think it is related to this line of code https://github.com/joomla/joomla-cms/blob/4.0-dev/build/media_source/system/js/fields/joomla-image-select.w-c.es6.js#L148-L150 . Maybe we should check and make sure there is width and height returned before adding lazy load. But that can be addressed in a different PR

So unless someone want to do a final code review, I think this PR can be set to RTC (It has been tested by @dgrammatiko and @richard67 for Local adapter, tested by myself and @chmst for dropbox adapter).

avatar dgrammatiko
dgrammatiko - comment - 24 May 2021

@joomdonation try this:

if (attribs.getAttribute('is-lazy') === 'true' && Joomla.selectedMediaFile.width > 0 && Joomla.selectedMediaFile.height > 0) {
  isLazy = ` loading="lazy" width="${Joomla.selectedMediaFile.width}" height="${Joomla.selectedMediaFile.height}"`; 
}

FWIW the adapter SHOULD return an object with predefined properties (width, height, url...) so if the dropbox is not returning those is kinda broken

avatar joomdonation
joomdonation - comment - 24 May 2021

@dgrammatiko Will do that in a separate PR. For some reasons, RIPS does not like this PR, so I will stop making un-necessary change to this PR :).

avatar joomdonation
joomdonation - comment - 24 May 2021

For dropbox, maybe it could not detect with and height of the image somehow. Or developer did not put much effort into that plugin. They created the plugin to test the adapter concept only :)

avatar dgrammatiko
dgrammatiko - comment - 24 May 2021

I'm leaving this here:

const getImageSize = (url) => {
  return Promise((resolve, reject) => {
    const img = new Image();
    img.src = url;
    img.onload = function() {
      resolve({ width: img.width, height: img.height });
    };
    img.onerror = function() {
      reject('Image failed to load');
    };
  });
}

// Can be used like:
const { width, height } = await getImageSize('https://...');
avatar richard67
richard67 - comment - 24 May 2021

Regarding the dropbox plugin I think the PHP error is related to or is the one described in this issue: Digital-Peak-Incubator/plg_filesystem_dpdropbox#2 .

If I rename the "guzzlehttp" folder in the "vendor" folder of that plugin to something else, the PHP error is gone, and I see "Dropbox" and "Local" filesystems in the media manager.

For articles I can select and then change selections using the "Dropbox" file system.

avatar richard67 richard67 - test_item - 24 May 2021 - Tested successfully
avatar richard67
richard67 - comment - 24 May 2021

I have tested this item successfully on cfa5bd2


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

avatar richard67 richard67 - change - 24 May 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 24 May 2021

RTC


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

avatar dgrammatiko
dgrammatiko - comment - 24 May 2021

@chmst could you test this pr but with the script from #34177 ?

@joomdonation is there anything more that needs to be changed in the selector script?

avatar joomdonation
joomdonation - comment - 24 May 2021

@dgrammatiko The only thing I want to change is if the width and height is zero, it should not be added to lazy loading. So basically, I think all I want is the code you propose at #33724 (comment)

Just for your information, I could not say that I fully understand media api. However, I remember I see from adapter interface that adapter should return width and height of the image, so I'm unsure if detect width and height using js like you did in #34177 is needed. I think adapter will have to return that information using server side code. We just got the problem here is because the dropbox adapter is not a complete plugin, as I said, it was created quickly for testing purpose. So maybe we don't have to handle that special case for now.

avatar dgrammatiko
dgrammatiko - comment - 24 May 2021

I remember I see from adapter interface that adapter should return width and height of the image

We all know by now that any Joomla API is totally cannibalized from some devs thus the code to sniff the size in case the adapter is not respecting the API. Anyways, the code is ~10locs so I guess it doesn't change anything (it will only run if width or height is 0 which should be very unlikely if devs are coding correctly their adapters)

avatar joomdonation
joomdonation - comment - 24 May 2021

it will only run if width or height is 0 which should be very unlikely if devs are coding correctly their adapters

If so, I guess it's fine/

avatar rdeutz rdeutz - close - 26 May 2021
avatar rdeutz rdeutz - merge - 26 May 2021
avatar rdeutz rdeutz - change - 26 May 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-26 10:14:03
Closed_By rdeutz
Labels Added: ? ?
Removed: ?

Add a Comment

Login with GitHub to post a comment