tractorcow-farm / silverstripe-fluent

Multi-language translate module for Silverstripe, without having to manage separate site trees.
BSD 3-Clause "New" or "Revised" License
93 stars 111 forks source link

FluentFilteredExtension applied on files breaks asset-admin. #114

Open bummzack opened 9 years ago

bummzack commented 9 years ago

I tried to use the FluentFilteredExtension on File, so that I can filter files within the File Edit dialog in the CMS. This works fine, but it breaks the CMS File admin.

Steps to reproduce:

Edited by @robbieaverill: Note that I've reopened this issue to track asset-admin support for FluentFilteredExtension on File/Image models in SilverStripe 4.

tractorcow commented 9 years ago

Hm, that seems to be a real problem! It's a tougher challenge since files aren't simply database entities, but have physical presence.

bummzack commented 9 years ago

Why do you think the physical presence of files changes things? Do you think the file-admin will then only show files that are "filtered" for a specific locale?

I wasn't able to dig into the code much, but the error I'm getting seems to indicate that there's something wrong with the formfield-generation. To me it looks like the locale checkboxes are being added multiple time:

user_error(collateDataFields() I noticed that a field called 'LocaleFilter_de_DE' appears twice in your form: 'Form_EditForm'.  One is a 'CheckboxField' and the other is a 'CheckboxField',256)
bummzack commented 9 years ago

Now that I think of it, it's probably a bad idea to add the FluentFilteredExtension to the File dataobject... because:

Applying the FluentFilteredExtension to Image works fine and if it would work the same way for files it could be beneficial (for example to filter PDFs). I think the main issue here is that a Folder is also a file which seems to break the file-admin.

tractorcow commented 9 years ago

Why do you think the physical presence of files changes things? Do you think the file-admin will then only show files that are "filtered" for a specific locale?

Mostly because any 'sync filesystem' action could potentially fail if a database record exists for a file, but isn't viewable in the current locale. There isn't any way of filtering the visibility of the physical files via locale.

tractorcow commented 9 years ago

Applying the FluentFilteredExtension to Image works fine

You need to apply the extension to the base class (in this case, File) otherwise it won't work properly. :)

The following will include images that are filtered out in any locale.

File::get() /* has no knowledge of extensions applied to subclasses, so those will be ignored */
tractorcow commented 9 years ago

Sorry if it sounds like I'm party pooping... I think that the extension has a lot of work still to go before it's ready for files.

Perhaps a subclass FileFluentFilteredExtension, to handle the file-specific behaviour? (e.g. ignoring folders, etc).

bummzack commented 9 years ago

Ok I guess I understand now. Seems to be simpler to use a DataObject wrapper for files in this case.

For my use-case it was enough to apply FluentFilteredExtension to Image, but you're right, this isn't a solution that will work with all use-cases.

robbieaverill commented 6 years ago

Closing due to inactivity, feel free to reopen if this is still a problem

micahsheets commented 5 years ago

I have a project that really does need to be able to Filter files based on Locale at least on the front end. I have products that are shown in multiple languages and a product has a PDF instruction documents. I want to be able to attache all the instructions in all languages in the CMS and have only the document with the currently viewed local show on the front end. This works for other DataObjects with FluentFilteredExtension but when I add that extension to Silverstripe\Assets\File, the Files Manager in SS 4 does not show the Locales dropdown so I can select what local any given file is. Is this the current state of the extension or is this a bug I am running into?

robbieaverill commented 5 years ago

Thanks for posting this @micahsheets. This is a wider issue of the fact that React forms don't use getCMSFields(), instead using the new PHP form factory and associated API methods. While this issue is closed, there's a new issue covering this with some detailed background information and also a suggested workaround/fix: https://github.com/tractorcow-farm/silverstripe-fluent/issues/491

Feel free to make a PR into this module if you come up with a working patch you'd like to contribute =)

robbieaverill commented 5 years ago

Apologies, #491 is related but not the same. I'll reopen this so we can track it again.

micahsheets commented 5 years ago

I wrote an Extension to FileFormFactory which has the updateFormFields function adding a ListboxField to the File edit form in asset manager. This does work if FluentFilteredExtension has been added to the File class. The only real issue is that all files that already exist in the database have to be set to one or more Locales or they will not show up at all. There is no automatic fallback for Fluent filtering like there is for Site tree content.

micahsheets commented 5 years ago
class FluentFilteredFileExtension extends Extension
{
    public function updateFormFields(FieldList $fields)
    {
        $locales = Locale::get();

        // If there are no Locales, then we're not adding any fields.
        if ($locales->count() === 0) {
            return;
        }

        $fields->insertAfter(
            'Title',
            ListboxField::create(
                'FilteredLocales',
                _t(__CLASS__.'.FILTERED_LOCALES', 'Display in the following locales'),
                $locales->map()
            )
        );
    }
}