#21189 solved the issue with inserting some images by hardcoding the adapter path. Whilst that fixed an immediate issue it's absolutely not good for when we support images hosted in the cloud.
No need to hardcode adapter meaning we can't use 3rd party adapters
Hardcoded adapter.
Labels |
Added:
?
?
|
There isn't a URL field in j4. So yes - but I've no clue if that's going to continue working anyhow - needs a separate issue
I wonder why do we have this fancy adapters, instead of use a stream wrapper PHP feature?
https://www.php.net/manual/en/class.streamwrapper.php
https://www.hashbangcode.com/article/php-custom-stream-wrappers
In my opinion, there are three options:
&path=local-0:/' . $folder
I would prefere the 3rd option
There is a 4th option: do something similar to the lazyloading approach, eg attach the adapter as a url param
BUT essentially the problem is the media field itself:
There is a 4th option: do something similar to the lazyloading approach, eg attach the adapter as a url param
Yes, but then you have to attach the adapter + the internal URL, otherwise you're running into the 2. solution and have to recalculate it (which is not always possible).
BUT essentially the problem is the media field itself:
* it stores the data as a string although a rich format (json) will be more appropriate * although it implies that all media types are covered truth is that in reality it is an image field
True, question is, if we should do a solution for 4.0 or don't rush it and do it for a later version (I would prefere)
Who is designed this adapters in general?
Check how it works in drupal (public://, private://, temporary://, foobar:// stream wrappers).
Store the file with so called adapter, and resolve a real path while rendering, no need json or other fancy c.. stuf.
True, question is, if we should do a solution for 4.0 or don't rush it and do it for a later version (I would prefere)
It doesn't have to be rushed, let me explain.
The media manager needs clear signal on the media types, right now this is extremely convoluted (there is one field that holds all the allowed types) but it needs a more granular approach. I already laid the foundation for this with https://github.com/joomla/joomla-cms/pull/31233/files
Then the rest is just either one or multiple fields (eg MediaObject or MediaImage, MediaAudio, MediaVideo, MediaDocument)
and some appropriate changes on the JS selector (again the foundation is laid by separating the filed modal from the selector part https://github.com/joomla/joomla-cms/blob/4.0-dev/build/media_source/system/js/fields/joomla-image-select.w-c.es6.js )
In sort it can be done in relatively sort timeframe, my problem is that the PRs are sitting there without any tests for too long (mean time for merging > 2 weeks) so most of the times some conflict resolving is involved and that is really discouraging for any contributor
Store the file with so called adapter, and resolve a real path while rendering, no need json or other fancy c.. stuf.
@Fedik that will imply that you preg_replace (or whatever is needed to resolve the adapter) for content images and I thought you were against parsing the content for performance reasons...
that will imply that you preg_replace (or whatever is needed to resolve the adapter) for content images and I thought you were against parsing the content for performance reasons...
yes, you are right, however, it not always use of preg_replace,
Example stand alone field that store publick://blabla/path/to-image.jpg
can be resolved directly (that is all "media" fields).
And for rest of images (mainly in article) we already have a regexp, that need to adjust:
joomla-cms/plugins/system/sef/sef.php
Lines 118 to 136 in 3448c43
Or we have to insert "resolved" image in to editor, this may lead to other issues, example if "adapter" changes.
Or we have to insert "resolved" image in to editor, this may lead to other issues, example if "adapter" changes.
FWIW this is what happens right now both on any editor content but also for any media field url, the URL is always the resolved one.
Example stand alone field that store publick://blabla/path/to-image.jpg can be resolved directly (that is all "media" fields).
I'm not sure that I understand what you mean by saying can be resolved directly
. Can be resolved when the img is echoed (how) or something else (redirect)?
Can be resolved when the img is echoed (how) or something else (redirect)?
yes, when echoed:
echo FancyAdapterHelper::resolve($article->intro_image);
btw, there another issue with adapters, it should not be local-0:/
, local-1:/
... local-x:/
, they at least should be "named".
If user change ordering in the plugin parameters, this will break all stored path.
Hm, I think because of this issue, my idea with "resolving while render" makes no sense.
yes, when echoed:
That's a no for me. An external adapter to resolve the url will need a request and that is definitely not performant (a page with 3 images will render after sending 3 requests (sequential because PHP is not async) and waiting for as long as these requests send back the appropriate responses. Sorry but realistically that's not a great way forward...
If user change ordering in the plugin parameters, this will break all stored path.
AFAIR it's not down to the plugin order but rather the plugin ID. So local-0
is coupled to plugin local, id 123 (or something like that)
render after sending 3 requests
what requests? it just:
<img src="<?php echo FancyAdapterHelper::resolve($article->intro_image); ?>" />
instead of :
<img src="<?php echo $article->intro_image; ?>" />
it's not down to the plugin
it IS plugin issue, parameters ordering, not plugin ordering:
joomla-cms/plugins/filesystem/local/local.php
Lines 92 to 103 in eb29c45
0 - is what is first item in array.
what requests? it just:
Sorry you're missing the point, eg to resolve this: aws-s3://blabla/path/to-image.jpg
you need to get the REAL URL from AWS S3. That needs a request/response. Check the demo adapter: https://github.com/Digital-Peak-Incubator/plg_filesystem_dpdropbox/blob/81424fc3671bfb8b9b1e63ad6bced8f8874dd95e/src/Adapter/DPDropboxAdapter.php#L151
it IS plugin issue, parameters ordering, not plugin ordering:
Well
PS wondering if this
$adapters[$directoryEntity->directory] = new \Joomla\Plugin\Filesystem\Local\Adapter\LocalAdapter($directoryPath, $directoryEntity->directory);
could solve the problem
Sorry you're missing the point, eg to resolve this: aws-s3://blabla/path/to-image.jpg you need to get the REAL URL from AWS S3. That needs a request/response.
remote stuff must be cacheable, or whole "adapters" idea is not much usable
PS wondering if this $adapters[$directoryEntity->directory]
in theory could work, but I do not know what to do if user enter directory blabla/my/images
in theory could work, but I do not know what to do if user enter directory blabla/my/images
You could hash it with md5 or something similar. Unique identifier is what we're after here imo. Btw this needs few changes in the vue application as well
yeah, or allow User to enter the "suffix" name (with CMD filter), and do not allow to edit it once it saved :)
hm, or, do not allow to edit whole thing once it saved, allow only add new
Labels |
Removed:
?
|
Because it says "alternative solution" makes me thinking we have a solution, so it works. So what is blocking us from a release?
Any 3rd party plugins developed will never show in the article view because currently we are hard coding the local one. It likely means database changes which IMO probably best done in the major release version for safety
Labels |
Added:
?
|
Bringing Release Blocker label back. My understanding is that we couldn't fix this after RC/Final so we should find a solution before RC
@bembelimen and I discussed this yesterday again and came up with 2 additional solutions/variants.
Minimal invasive, adding the virtual path as #jmm:path
to the image and use the SEO plugin to remove it in the onPrepareContent
event (similar to the suggestion by @dgrammatiko). This would maintain full b/c 3rd party plugins and solves the issue in a simple way. But this is not really future proof.
Adding an additional hidden field (ex. $name . '_metadata'
) with the virtual path saved as json (this would allow us to save other meta data too in the future), Benjamin pointed out that the field is mostly used in an input array (com_content uses jform[images][image_intro]
), so for core this should be no problem (if we really only use array parameters saved in json fields). For 3rd party components we have a good chance that they also use an array and save it as json in the database. In case of a normal database field, Joomla would simply not save the additional field and the user starts in the media manager root folder. Of course if we add new parameters this have to be optional or we make sure 3rd party extension developers get notified that they need to support the additional json field.
Feedback is welcome by anyone.
Although I totally support the JSON blob solution the 2nd solution is not optimal, we have better tools (web components) fo this task. Also the problem is not actually solvable if the adapters don't become named
instead of array index, eg #26750 (comment)
In short my solution here would look something like:
<field
name="imageurl"
type="media"
label="COM_BANNERS_FIELD_IMAGE_LABEL"
directory="banners"
adapter="local"
hide_none="1"
size="40"
/>
<joomla-field-media class="field-media-wrapper"
type="image" <?php // @TODO add this attribute to the field in order to use it for all media types ?>
base-path="<?php echo Uri::root(); ?>"
root-folder="<?php echo ComponentHelper::getParams('com_media')->get('file_path', 'images'); ?>"
url="<?php echo $url; ?>"
modal-container=".modal"
modal-width="100%"
modal-height="400px"
input=".field-media-input"
button-select=".button-select"
button-clear=".button-clear"
button-save-selected=".button-save-selected"
preview="static"
preview-container=".field-media-preview"
preview-width="<?php echo $previewWidth; ?>"
preview-height="<?php echo $previewHeight; ?>"
adapter="local"
value="{"adapter": "local" ... }"
></joomla-field-media>
With the adapters there a multiple issues.
One of is #21189, default "folder", and selecting the value.
After looking at it again, I think we can fix it in this way without b.c:
1 Need a global config for default Adapter. Similar to Editor, Captcha.
2 If media field have a path without adapter we use "default adapter" otherwise we use the requested adapter.
Then it will work like next, example we have default adapter local-0
, and the media fields is:
<field name="i1" type="media" directory="my-folder" label="foobar" />
<field name="i2" type="media" directory="dropbox-0://my-folder" label="foobar2" />
Then in modal for the first field will be shown local-0://my-folder
, and for second one is dropbox-0://my-folder
. Without hardcoding. Similar to what @dgrammatiko just wrote, but not exactly.
Another issue about storing the value.
For the media field, it can be stored with adapter path, eg: dropbox-0://my-folder/foobar.jpg
.
I do not see a big reason to use json for it, it already contain everything we need to get a real path.
The path later can be resolved while rendering (without onPrepareContent
and #jmm:path
):
<img src="<?php echo FancyAdapterHelper::resolve($article->intro_image); ?>" />
Important note here that AdapterHelper::resolve
should take care about caching metadata.
Or just store resolved path.
The potential issue with storing json in $name . '_metadata'
and resolved path can be, example, when Dropbox/Aws/Etc token/account changes, then the old stored values may be not valid anymore, and will be no easy way to recover, or clear that cache.
Unless I missed something.
Similar to what @dgrammatiko just wrote, but not exactly.
Actually reusing the existing directory
attribute is better
Or just store resolved path.
Because the adapters are not caching their data this might be the only possible solution right now. Unless if someone is willing to create a store (even a JSON file should be ok-ish as a starting point)
Anyways I think in order to move forward we need to convert the adapter part from array-indexed based to name-based. The rest shouldn't be too hard to tackle
Sorry guys but I think you completely missed the point.
We need to save the adapter in the database, it can't be in the xml. I had another talk with @bembelimen and finally we didn't see a proper way to save the metadata in it's own database column.
Additional a json value for the image form value is a no-go for me. Because this would require to parse json in frontend and may load the jformfieldmediaform (I know it uses namespaces) to do this.
This would also break every template using mediaimage and component using this field. They all need to be changed.
At the moment I see solution 1 the only possible option without a b/c problem and without making some really strange moves in JFormField and JForm.
Using the directory parameter in joomla-field-media is of course the right move, but joomla-field-media must set the value to something like /url/image.jpg#jmm@local-1://path/to/image
or /url/image.jpg?jmm_adapter=local-1&jmm_path=path/to/image
Changing the index to name is also a must I think but that's something handled in the adapters right?
Maybe it's not needed if the value of local-1
is part of the adapter like local-/images
don't know if this work.
Using the directory parameter in joomla-field-media is of course the right move, but joomla-field-media must set the value to something like /url/image.jpg#jmm@local-1://path/to/image or /url/image.jpg?jmm_adapter=local-1&jmm_path=path/to/image
That is fine and you only have to extend the existing code:
joomla-cms/libraries/src/HTML/HTMLHelper.php
Line 671 in 7cd0361
That said there's still a problem with setting a field to open on a particular adapter, path and honestly I thought this was what was this issue all about...
Here is 2 issue, need to clarify
1 select value in the modal
2 store selected value
You talking about saving the value.
We need to save the adapter in the database, it can't be in the xml.
Yes, and no
Here I talked about pick up the value in modal.
The xml need to know what adapter is default for directory="foobar"
, that is a reason why we have hardcoded
path=local-0:/' . $folder
in #21189 . The field itself does not know which adapter to use, so we need to have a default one and allow to use another adapters with directory="dropbox-0://foobar"
.
It is nothing to do with saving the value, just about selecting the value.
And about actually the saving the value, it a bit hard.
I agree we need to store the adapter, somwhere.
Your /url/image.jpg#jmm@local-1://path/to/image
not that bad idea.
Maybe simplify to /resolved/path/to/image.jpg#local-0://path/to/image.jpg
,
for external adapters it will be http://dropbox-foobar.com/blabla/hash/path/to/file.jpg#dropbox-0://path/to/file.jpg
I am a bit afraid of this: ...#jmm@local-1:...
:)
Changing the index to name is also a must I think but that's something handled in the adapters right?
yeah, it handled by adapters,
I think I can live with that, for now :)
Using the directory parameter in joomla-field-media is of course the right move, but joomla-field-media must set the value to something like /url/image.jpg#jmm@local-1://path/to/image or /url/image.jpg?jmm_adapter=local-1&jmm_path=path/to/image
That is fine and you only have to extend the existing code:
joomla-cms/libraries/src/HTML/HTMLHelper.php
Line 671 in 7cd0361
(by the way the return is an object because I already thought of the adapter and the next one a hash for cache invalidation)
That said there's still a problem with setting a field to open on a particular adapter, path and honestly I thought this was what was this issue all about...
Is this function already used in the frontend, if so you should be right extending it would be enough maybe cleaning the parameters in the seo plugin would be good for b/c but not a must at this point.
So what do we need, following the already existing parameters we need &joomla_image_adapter=local-1&joomla_image_path=/path/to/image
, or we stick it together with an uri schema.
@Fedik you are right creating new parameter style doesn't make sense keep the introduced semantic.
That should be filled by joomla-field-media to the value right? and on load of the jformfieldMedia it should be provided to joomla-field-media as attribute.
Since my JS knowledge is not as good as yours @Fedik or @dgrammatiko it would be great if you can create the PR.
It would be optimal if you can build this soon because RC is on it's why and this have to be fixed before. (also the adapter order is a problem, didn't looked into this in deep yet).
both lines are wrong ;-) should be HTMLHelper::_('cleanImageURL', $image->image_intro);
It's also used in th HTMLHelper::_('image') so hopefully core and 3rd party uses this function to create images in this use case. If not we still can extend the seo plugin. @Fedik would be awesome if you can create this pr.
I hope there is a way to not change the value which is set for the input after the image being selected to avoid me (third party developer) have to change the code to render the image
As of right now, that value is stored into database and I only have to append JUri::root(true) to that value to render the image
Actually you should not need to append JUri::root(true)
now. This would also break adapters that return a cdn link (not sure if this is tested already). For the 'local' adapter this path should not change (that's the reason for the discussion in this issue) for 3rd party this could be a full URI. We don't have control which get return by a 3rd party adapter (that's my understanding ymmv)
both lines are wrong ;-) should be HTMLHelper::_('cleanImageURL', $image->image_intro);
Of course, they are, because I wrote them...
If not we still can extend the seo plugin
Maybe that would be a cleaner approach. TBH I didn't know that the SEO plugin was manipulating the image URLs
As of right now, that value is stored into database and I only have to append JUri::root(true) to that value to render the image
That was one of my aims when I did the lazyloading, to be less invasive for the 3rd pd. Although database should be the only source of truth, removing/renaming an image in the com_media results in broken links, so there should be a better way to handle the URLs so the system is more friendly to the users, but that is a bit complicated although not impossible. The whole idea is to do the least amount of processing when displaying content in the front end, editing can be slower
Actually you should not need to append JUri::root(true)
So far, I only care about local adapter (it works in the same way with what we are using for our extensions in Joomla 3 - same codebase). And without JUri::root(true)
, the image won't be displayed when the site is used in sub-folder.
Depends on how it is implemented, I guess I can always find a way to make it works with my extension. I just want to express my concern here to minimize the backward incompatible changes for third party extensions developers.
And maybe most sites will use local adapter ? We should keep that in mind while making the changes
&joomla_image_adapter=local-1&joomla_image_path=/path/to/image, or we stick it together with an uri schema.
yea, it also can be as param, it should be more easy to parse with parse_url()
&joomla_file_uri=dropbox-0://path/to/file.jpg
maybe it even more consistent than hash #
would be awesome if you can create this pr.
I will try, only it will not be that fast :)
yea, it also can be as param, it should be more easy to parse with parse_url() &joomla_file_uri=dropbox-0://path/to/file.jpg
I would be extremely careful with the ://
in a URL param, to be precise these need to be URL-encoded if I'm not wrong
that should not be a problem, anyway every url-param should be encoded for use in request URL
anyway every url-param should be encoded for use in request URL
Sure, the ones I added were just numbers so no extra step there, but this one needs a bit of care both on the js side and also on the PHP world, thus my comment
I tried to download and install Dropbox adapter and see that when we select an image, the full URL of that image is also returned. So a quick question comes in mind: Should we store that full image URL into the value of the media field so that we don't have to query adapter again to get the URL each time we want to display the image? That would simplify our work and also better performance. Not sure if it could cause any problem?
Not sure if it could cause any problem?
you wouldnt want to do that for local adapters
Should we store that full image URL into the value of the media field so that we don't have to query adapter again to get the URL each time we want to display the image?
I think this is the only viable solution atm (I also came to the same conclusion a few months ago #26750 (comment)). The downside of storing all the relevant info in the db as a string means it needs some manipulation before using it, but changing the format from string to JSON I think is way too invasive right before RC. I guess we can live with a little PHP helper.
you wouldnt want to do that for local adapters
We are not doing it at the moment and probably in the future. The image selector already transforms on the fly the local adapter to a relative path
Not sure if it could cause any problem?
you wouldnt want to do that for local adapters
Yes, of course. We would only do that for external adapters only
you wouldnt want to do that for local adapters
Yes, of course. We would only do that for external adapters only
As I said we're good, I'm not that bad on building things:
joomla-cms/build/media_source/system/js/fields/joomla-image-select.w-c.es6.js
Lines 110 to 125 in 9c6d53a
@dgrammatiko Assume we go with that approach, do we have a way to use a different value for preview image of the media field? As of right now, we are using value for preview image, too, and from my quick test with Dropbox, it won't work because Dropdopbox does not allow us to append query string to an image URL. So if we want to append query string to the URL to save to database, the preview image won't be displayed (there is some error in browser console)
Since the adapter return thumb image, too, would be great if we can use that as preview image
As I said we're good, I'm not that bad on building things
I knew that. I had a quick look at the current code already (although I only have basic JS skill :D )
do we have a way to use a different value for preview image of the media field?
Actually, the code, in the first version was satisfying this requirement but to be honest here I haven't tested the dropbox plugin lately (or to be more precise I didn't use it when I refactored the code). It should use the thumb
eg Joomla.selectedMediaFile.thumb = resp.data[0].thumb_path;
for the preview and the url
for the value. If it's not doing that ping me and I will fix it
I was hoping it works like that, sadly, it is not. So I guess we will need your help for fixing that. Joomla.selectedMediaFile.thumb
seems not used for anything at the moment.
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-14 06:26:28 |
Closed_By | ⇒ | joomdonation | |
Labels |
Added:
?
Removed: ? |
Labels |
Removed:
?
|
As stated in that PR it also means that we can't type a url in a field url