October: Add support for maxItems option to MediaFinder

Created on 16 Dec 2015  路  53Comments  路  Source: octobercms/october

Hello,
Media finder could not pick multiple images.

Am i wrong?

Medium Accepted In Progress Enhancement

Most helpful comment

I understand that register path in a string when you deal with just one image make sense for backward compatibility, but it would be great to provide a specific options for that, a "conserve legacy behavior" checkbox, checked as default.

If unchecked, it would save image path in an array, even if there is just one image, the advantage to do that is that you have consistency over the way informations is stored, you can change the number of max items in the backend without changing your frontend code.

All 53 comments

Hi
In his models tries to use that way "$attachMany":

public $attachMany = [
    'featured_images' => ['System\Models\File', 'order' => 'sort_order'],
];

i tried this but i can choose only one image

Do you use 'featured_images' for the field at file fields.yaml

It works like this:
$attachOne = 1 image in the attachment
$attachMany = many images in the attachment

try that:

public $attachMany = [
        'my field name' => ['System\Models\File'],
];

did you try that ?
if i remove
type: mediafinder
it works but mediafinder widget attach only one image.

if you prefer to send me your plugin, I will try to fix it.

@chesterx Current version of media finder widget allow to only choose one file.

@Hatalias He is talking about media finder widget not file upload widget. Those two widgets works differently.

http://octobercms.com/docs/backend/forms#widget-mediafinder

http://octobercms.com/docs/backend/forms#widget-fileupload

The media finder is a basic widget that only returns a string path for a media library item. I suppose it would be possible to use the repeater widget with a mediafinder widget inside to allow for multiple selections.

If I pick a folder instead of one file, how can I scan all files in this folder within the controller? Could this be another solution?

@daftspunk it would be awesome if there were 2 or more selected images in media finder - then it would return an array of paths, and we could store in DB row type json.

Wouldn't if fix the problem?
Or there are some issues with such approach?

Thanks!

@roulendz I could see that working, but with a limitation. I think the developer would need to specifically enable that mode through an option (like multiple: true or something like that) because you would now have two different types of data being saved by one widget that requires two different types of database columns in order to store them correctly.

The multiple option would then trigger requiring a jsonable attribute on the model to save the value to while the default configuration would continue saving string paths.

Sounds good. Perhaps a maxItems option (default: 1) would be enough to imply multiplicity -- did I use that word correctly? Setting to null would imply infinite.

I like that even better @daftspunk

Need to also implement #2130 in this when this gets added.

This functionality interests me

Also interested, what I do actually is to create 2 image fields per content type.

  • One "Image" with mediafinder widget
  • One "Images" with fileupload widget

Then, depending on the need of the customer (single or multiple upload), I disable the other image field.

It's quite annoying in term of UI and development to have to deal with multiple fields for the same things.

In addition, the file upload widget does not allow to browse the files on the server. So people that needs to use the media manager are stuck with only one image to illustrate the content.

I know the solution of putting a media manager field to a repeater, but the resulting UI is really bad, customers don't understand why it's so complex to add some images to the website (compared to the file upload system).

For the solution, you could simplify the process and directly define a json variable even if there is only one image. Then you loop throught your image using a foreach, if there is only one, there is no problem.

Whereas if you change from string to json if there are several image, you have to change your code on frontend or you will have some errors, but I understand than keeping single string for single image would make the system compatible with precedent versions.

I begin to make october cms website more and more regulary and this feature is probably one of the most needed, so a big +1 for me.

@Alex360hd this is a planned feature, I will be doing some work on the MediaManager this September, so perhaps I'll be able to work this into those improvements.

@LukeTowers any update on this ? Could you work on the MediaManager in September ?

@Alex360hd it's been pushed back till October but it is still a planned feature.

Hi, I saw the board about the media finder improvements and it's a good thing !

The other day, I noticed something annoying that could be added to the board :

When you use the standard file upload system for images, you can click on them to add a title and a description. It's very useful when you want to manually control the alt text.

Unfortunately, when you use the media finder, you can't click on the chosen image in order to add a title and a description.

The only fix I found for that is to put the media finder, with a textfield for alt image in a repeater, but the resulting UI is not that good, plus you can't block the user to add more than one image.

I know this is probably because the media finder only register the path to the file as a simple string. Would it be possible to consider to save the file as a real attachment, like file upload widget, in order to be able to change some meta informations without being obliged to use the repeater ?

@Alex360hd if you look at that board's description, you'll see that is actually the main prompter of all these improvements, supporting metadata on Media Library images. That to say, don't worry, it's coming :)

Hi @LukeTowers can I help you to improve mediafinder widget, for adding multiple images?
Is there any specification for it? Or works on this improvement is not in progress now?

@samorai work on this was supposed to start in September when a potential sponsor for the project was hoping to land a client project to pay for it. That's looking unlikely to happen at this point so I'll be working on it during the time I have for non paying work. It's still planned, just at a slower pace.

The specification for this particular aspect is as follows:
Add a new config option to the mediafinder widget "maxItems" defaulting to one. When maxitems is set to one, a string is returned for saving (the image path) to maintain backwards compatibility. When maxitems is over 1, an associative array is returned in the following structure

[
    ['path' => '/whatever/image.jpg'],
    ['path' => '/whatever/image.jpg'],
]

When maxItems is over 1, the returning data should be saved to jsonable column, yes?
@LukeTowers

I understand that register path in a string when you deal with just one image make sense for backward compatibility, but it would be great to provide a specific options for that, a "conserve legacy behavior" checkbox, checked as default.

If unchecked, it would save image path in an array, even if there is just one image, the advantage to do that is that you have consistency over the way informations is stored, you can change the number of max items in the backend without changing your frontend code.

I totally agree with @Alex360hd idea. We could ensure downward compatibility (in most cases) if we would make the |media filter aware of this behavior and let it handle string and array:

<!-- old markup pathVar is string -->
<img src="{{ pathVar|media }}">
<!-- old markup but pathVar is an array -> take first array element as path. If empty array return empty string  -->
<img src="{{ pathVar2|media }}">

<!-- new markup new pathVar2 as array -->
{% for image in pathVar2 %}
<!-- image is path as string -->
<img src="{{ image|media }}">
{% endfor %}

@munxar and how do we we support old schema that stores paths in a VARCHAR column, not a jsonable one? I'm not opposed to the suggestion, just want to make sure that everything is covered. In theory we could check if the maxItems property is actually defined and if so have that behaviour occur. Also, how can we keep the same frontend code if legacy columns untouched by the change yet are pure strings and new ones are arrays?

@LukeTowers To your first question: Support old schema when reading the data and make a [oldDate] out of it. Saving models convert arrays with single entries to string.

Idea: And what about not touching the MediaFinder at all, and implement a new Widget that behaves like described: MultiMediaFinder?

[Side note: The existing MediaFinder should have been named MediumFinder ;-)]

@munxar that doesn't solve the issue with the front end or backend actually. We can handle old data no problem when we load it in the widget, but what we can't do is change data on records without even having them loaded by the backend at all. We also can't automatically convert old data to new data because presumably it's stored in a VARCHAR column with a default limit of 255 characters, so we can't try to automatically start shoving JSON in there instead.

@LukeTowers see my Idea above (edited while you posted)

Idea: And what about not touching the MediaFinder at all, and implement a new Widget that behaves like described: MultiMediaFinder?

@munxar that's what I'm saying doesn't solve the problem. How would a separate media manager solve the problem?

@LukeTowers Imho it does.
Existing backend MediaFinder widget -> keep as is
If I need multiple images, use the (new) MultiMediaFinder, it needs a jsonable column, it needs new markup in the frontend, but it will not break anything existing.
And it solves the initial need for a 'MediaFinder' that can support multiple images. It is just a new widget instead of putting this feature in the old one.

@munxar what's wrong with changes to the existing one? It would require the developer to touch the code to take advantage of it anyways (setting maxItems) so we can tell them to make the necessary changes to their code and schema at the same time. I'd rather find a simple way to handle old data then branch out the mediafinder widget, that adds unnecessary complexity to the system.

@LukeTower Not sure to understand, actually, Json variables (like repeater) are stocked in a TEXT column type on the database side (JSON column type is not supported by all systems, that's probably the reason), so it shouldn't be a problem to save a string or an array to that column.

A new widget is also a valid idea, I would prefer to have small widget type but all configurable (in fact I would like to merge file upload and media finder and consider consider the differences between the 2 as settings and parameters but it's my personal opinion)

@LukeTowers I understand, keep it in the existing one.

If the new MediaManager would write string or string[] depending on maxItems to the db we would have to write twig that must check for iterable and that is resulting in view markup like:

{% if pathOrPaths is iterable %}
    {% for path in pathOrPaths %}
        <img src="{{ path|media }}">
    {% endfor %}
{% else %}
    <img src="{{ pathOrPaths|media }}">
{% elseif %}

On the other hand we could always write string[] into the db, but allow for reading strings and convert them to string[] as far as the backend widget is involved. On the model we need a $jsonable column. Again a breaking change.

For the frontend it is not a breaking change, if we extend the |media filter (support string or string[] as argument). Because old code would most likely look like this:

   <!-- path is an array or a string -->
   <img src="{{ path|media }}">

New twig code looks like this and can handle zero to many images:

    {% for path in paths %}
        <img src="{{ path|media }}">
    {% endif %}

So to simplify, we have two options:

  1. write string or string[] to the db depending on maxItems
    -> results in more complicated twig markup, but doesn't change model data.
  2. always write string[] independent on maxItems
    -> simple twig, but would write new data always as array.

But there is a third option to have best of both worlds:
a) don't make the column jsonable but write string and string[] depending on maxItems
b) implement a new |medialist filter to decode the passed string to an array (if possible)
The resulting markup would be something like this:

{% for path in paths|medialist %}
    <img src="{{ path }}">
{% endfor %}

If paths is empty, |medialist would return [], if paths is a simple string value it would return [value].

What do you think?

@munxar Sounds good to me, it seems to mix best of both world

@LukeTowers I think, that idea with MultiMediaFinder is better solution. We can create them and set MediaFinder as deprecated.

@Samorai @munxar @Alex360hd I'm going to say that a different formwidget is a last resort.

The frontend compatibility is not difficult to solve as mentioned with a change to the media filter that detects an array and gets the correct value from it. One thing I'd like to point out is that my ideal data storage structure for this change is

$items = [
    [
        'path' => '/path/to/image.jpg',
        ...
    ],
    [
        'path' => '/path/to/image2.jpg',
        ...
    ],
];

The above structure is to allow for the theoretical overriding of media item metadata (not implemented yet) on a per field basis.

The backend compatibility could be solved. VARCHAR columns are 255 characters. JSON encoding a mediafinder value would add 12 characters ({['path:'']}) to the value stored in the DB. That is relatively We could provide something like the following in the widget:

public function getLoadValue($value)
{
    if (is_string($value) && is_valid_json($value)) {
        $value = json_decode($value);
    } elseif (is_string($value)) {
        $value = [['path' => $value]];
    }
    return $value;
}
public function getSaveValue($value)
{
     // this would be slightly more complicated as we would need to determine if the property
     // it's being saved to is jsonable or not to decide whether or not to provide the value as a
     // JSON-encoded string or as the associative array that it will be.
}

This would maintain backwards compatibility by supporting loading the following data types:

  1. Existing 1-item paths as strings stored in DB VARCHAR columns
  2. Existing 1-item paths as JSON encoded data objects stored in DB VARCHAR columns
  3. 1-or-many-item paths as JSON encoded data objects stored correctly in DB TEXT or JSON columns
    and saving the following data types:
  4. 1-item paths stored in DB VARCHAR columns as JSON encoded data
  5. 1-or-many-item paths stored in DB TEXT/JSON columns as PHP arrays to be serialized by the model

Thoughts?

Any updates?

@yabasha any feedback on the ideas discussed in this issue?

@LukeTowers I would not change the database column type depending on the number of media you have. I would always consider them as json as the only difference between 1 media and N media is the number of items in the JSON/array. It's just a matter of a loop.

In addition, it should be easy to transform a 1 media field to N media field, so the database column should consider the more complex scenario => so JSON.

If all that procedure around VARCHAR is for backward compatibility, I would consider to keep the actual media finder as a legacy widget (not recommanded one for new plugins) and a new media finder that could deal with JSON column without fear.

@LukeTowers Can we please get this implemented? I'd be willing to put up some money as well!

@LukeTowers To give you some feedback as well:

I completely agree with your assessment of maintaining bc and I honestly don't see the need for a new widget, since the bulk of the code would basically be a duplicate. That's not the way to go, certainly not for this feature request. Doing so is one of the reasons other frameworks are bloated.

Please let me know how we can get this implemented asap, thanks!

Here's a thought on backwards compatibility:
maxItems can have the following values:

null: Infinite amount of items
integer: Limit selection to specified number of items
false: (Default value when option not provided) Behavior disabled completely, only store and load strings for single items

This would retain backwards compatibility without requiring anything to be changed in the frontend. If a developer doesn't change anything then their mediafinder data behaves exactly as expected. If a developer wants to take advantage of this functionality then they either set maxItems to null or an integer and that stores the data in the associative array format and requires them to loop over the values on the frontend.

Thoughts?

@datune how much money could you put up? Perhaps @munxar, @Alex360hd, @yabasha, and @Samorai could pitch in as well and you guys can sponsor the functionality getting added.

If you guys can come up with $600 then I'll take care of this as a sponsored enhancement; otherwise it'll get added when I get around to it or someone else makes a PR of acceptable quality.

@LukeTowers Like discussed, I will sponsor this enhancement. Please feel free to implement this as you feel is best, and let me know if there is anything I can do to help. Thank you Luke!

Any progress?:)

@dimensi life's been crazy over the month but it's still in progress. Work is being done in the https://github.com/octobercms/october/tree/sponsored/mediafinder-maxitems branch.

Ahhh life... the most surprising repository :)

Sorry, it's simply taken too long. I understand life can get in between things, so no hard feelings!

@datune still going to get to it, I'll ping you when it's done

@LukeTowers

Has this multi mediafinder feature been implemented?

@MattMcManis not yet, life got in the way, @datune no longer wanted to sponsor it because it was taking a while so now it's dropped down my priority list. It's still in progress, however not on the top of my list.

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

Was this page helpful?
0 / 5 - 0 ratings