Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core fieldable file entity bundles #2632

Closed
10 of 14 tasks
Al-Rozhkov opened this issue Apr 9, 2017 · 82 comments · Fixed by backdrop/backdrop#2838
Closed
10 of 14 tasks

Core fieldable file entity bundles #2632

Al-Rozhkov opened this issue Apr 9, 2017 · 82 comments · Fixed by backdrop/backdrop#2838

Comments

@Al-Rozhkov
Copy link
Member

Al-Rozhkov commented Apr 9, 2017

Were working on adding all of the file_entity module to Backdrop core, including fieldable files.

Before PR merge:

  • Convert file type info to config (move from DB table)
  • Remove the default alt and title fields provided out of box on the Image file type.
  • Make UI changes to simplify the "Manage File Display" modes form.
  • Add the upgrade path from Backdrop 1.x
  • Test the upgrade path from Backdrop 1.x
  • Add an upgrade path from Drupal 7 for D7 file types and file_display settings.
  • Test the upgrade path from Drupal 7

Before release (if possible):

  • Move "Manage File Display" settings to the "Manage displays" tab (as settings for the "File" psuedofield.
  • Add a wrapper function for file_entity_access with watchdog_deprecated_function()
  • Integrate with Path module (add to vertical tabs + Provide default path aliases for files)

Optional UX improvements:

  • Remove the confirm form when disabling / enabling file types
  • Make the disabled types gray, and below the enabled ones.
  • File type config form: Mimetypes should be a checklist or something helpfully clickable like tokens.
  • Add the "Add file" link into admin bar?

At a later time

  • Discuss adding back Alt and Title fields
  • Add back-end breadcrumbs for front-end pages /file/%/manage and /file/%/usage, to allow easy navigation back to /admin/content/files
  • add something like inline entity form so that we can get additional file fields to show up in other places like node forms and rich-text editor forms.
  • there is no obvious way to add a video to a node, now that we allow uploading videos. Should there be an issue for that?
  • When looking at the list of files under /admin/content/files, there should be a "View" link under the drop button that takes you to the file page.
  • Similar to the above, the File::uri() function currently returns the path to the file. Perhaps this should be the path like /file/[fid], so it matches other types of entities? That'd be a substantial API change, maybe shelve that for 1.15 so we can discuss it.
  • The file admin listing shows a file usage count. That should be made into a link to the /file/[fid]/usage page now that we have that page.

#3971 #Original Issue

We do have this task in File management meta issue #1448, but I didn't find separate issue for it.

Looks like we can make file entities fieldable with just one line of code:
$entity_info['file']['fieldable'] = TRUE;

So I merged "file types" and "fieldable file types" in one issue. Feel free to separate them if I'm wrong.


#Advocates: @docwilmot @jenlampton


PR: backdrop/backdrop#1924 by @docwilmot
PR: backdrop/backdrop#2238 by @jenlampton
PR: backdrop/backdrop#2832 by @docwilmot
PR: backdrop/backdrop#2838 by @jenlampton

@Al-Rozhkov
Copy link
Member Author

Looks like it's better to solve this one first #213 (Move file entity info, managed file, and file usage functionality into File module).

@Al-Rozhkov Al-Rozhkov added this to the 1.8.0 milestone May 6, 2017
@Al-Rozhkov Al-Rozhkov self-assigned this May 6, 2017
@Al-Rozhkov
Copy link
Member Author

Please let's push this to 1.8.0 milestone. I'm going to dive deep into this and any help from experienced developers would be very welcome.

I'm going to look on code from D7 File Entity module and adapt it to Backdrop.

@daggerhart
Copy link

Hi @Al-Rozhkov, just wanted to check in and ask how is this issue going so far?

I'm thinking through this and related issues a bit and I'm seeing this as two separate issues:

  1. Core file entity bundles
  2. Field-able file entities

Making them field-able should be relatively straight forward (as OP suggests). So I'm interested in what you think about the core provided file entity bundles.

D7 File entity module provides these 4 bundles:

  • Audio - no additional fields
  • Image - additional "Alt Text" and "Title", both text fields
  • Document - no additional fields
  • Video - no additional fields

Seems reasonable to me. Think we should we aim for the same?

Have any other thoughts so far during your review of the D7 File entity module?

@daggerhart daggerhart changed the title Different fieldable file types Core fieldable file entity bundles Jun 2, 2017
@Al-Rozhkov
Copy link
Member Author

Hi @daggerhart! I'm working on it. And I'm going to provide raw PR in next couple days.

I agree that fieldable file entities could be a separate issue. Feel free to open it.

Also I like D7 File entity approach. I think these 4 bundles is good defaults for Backdrop. But we can add description field for each bundle as @jenlampton suggests in #2070 (Move the file description column to file_managed table).

@daggerhart
Copy link

Awesome, please let me know if I can help! I'm going to be working on related issues a bit this weekend.

@Al-Rozhkov
Copy link
Member Author

Please be merciful. It was huge task for newbie. And it definetly needs more work.

File types configuration mostly was borrowed from node types, everything else was taken from file_entity module.

Please test sandbox and look at the PR. I would be happy to get feedback from experts and continue my work on this.

@Al-Rozhkov
Copy link
Member Author

Al-Rozhkov commented Jul 10, 2017

Oh! First mistake. Alt and Title fields in image bundle wasn't created in the sandbox, but was on my local upgraded site. I will fix this.


UPD: Fields in place now. Sandbox is ready for manual testing.

@daggerhart
Copy link

@Al-Rozhkov awesome! I'll have to wait until next week to take a look, got a lot going on over the next few days. But thanks for tackling this, I'm looking forward to checking it out

@jenlampton
Copy link
Member

We are two weeks from code freeze for Backdrop 1.8. If anyone else could help with manual testing, I'd love to see this issue included in the next release :)

@olafgrabienski
Copy link

olafgrabienski commented Aug 24, 2017

I've had a look at the sandbox site, added a "License" field to the Audio file type on admin/structure/file-types/manage/audio/fields, made sure that the display of the new field is enabled on admin/structure/file-types/manage/audio/display. Then I added a file field (Format: Audio) to the Post content type and created a test post. I couldn't figure out if and where the "License" field is actually supposed to be displayed. On the Node edit form? (Didn't see it there.)

@jenlampton
Copy link
Member

But I couldn't figure out where the field is actually supposed to be displayed.

I think that's on file/FID

@olafgrabienski
Copy link

olafgrabienski commented Aug 24, 2017

I see, thanks @jenlampton! Have found it now, not on file/FID though but on file/FID/edit. Is that the supposed behaviour?

Update: (only) after having edited it, it was also displayed on file/FID, makes sense!

@olafgrabienski
Copy link

@Al-Rozhkov Unfortunately I'm not experienced with the Drupal File Entity module. Can you mention particular aspects of the PR which need manual testing?

@jenlampton
Copy link
Member

With 1 day until code freeze, I don't think this one is going to make it into 1.8. Bumping milestone to 1.9.

@jenlampton jenlampton modified the milestones: 1.9.0, 1.8.0 Sep 3, 2017
@Al-Rozhkov Al-Rozhkov removed their assignment Dec 23, 2017
@jenlampton jenlampton removed this from the 1.9.0 milestone Jan 4, 2018
@quicksketch
Copy link
Member

After getting this so close but not quite finishing it, we're going to give this one another 24 hours to get wrapped up for the 1.14.0 code freeze.

@jenlampton jenlampton removed their assignment Sep 3, 2019
@ghost
Copy link

ghost commented Sep 3, 2019

Testing latest PR:

  • The description for the Media Types field has a spelling mistake: "This is the list of media types (MIME types) which will be aassociated with this file type."
  • When I saw the file types UI, I thought "Cool, I can create a 'Slideshow Image' file type with caption, link, etc. fields instead of using a content type for this." But then the media types thing says not to use the same media type for more than one file type. So I guess that's not a valid use case...? (assuming you'll want to also upload non-slideshow images too)
  • The default Image file type has a media type of image/*. Adding the fact that you can use wildcards to the description for this field would be very helpful (I'd have had no idea this was an option otherwise).
  • Would it be possible to grey out media types from the 'Available media types' fieldset that are already used in a file type? (since you can only use each media type once)
  • What's the 'Manage file display' section for, how does it differ from 'Manage displays', and should there be help text explaining as much?
  • There's no default value for 'Manage file display' for default Image type (not sure about others)
  • How do you use files with content types? I can't see a way to choose a file type, and the File and Image fields look the same... The same with CKEditor.

@ghost
Copy link

ghost commented Sep 3, 2019

Don't fully understand how Files are supposed to work (see last point above) and don't want to read through entire issue, so will have to leave my testing there until someone can help me understand all this...

@docwilmot
Copy link
Contributor

Done some UI testing, only two issues to note

  • the file type "Manage file dsiplay" page, none of the radios are initially selected, even on newly created file types, so files display as a link when viewed at file/fid. I think it would be expected that images would default to image, and vidoes to video, and audio to audio.
  • there is no obvious way to add a video to a node, now that we allow uploading videos. Should there be an issue for that?

@jenlampton
Copy link
Member

There's no default value for 'Manage file display' for default Image type

the file type "Manage file dsiplay" page, none of the radios are initially selected, even on newly created file types

@docwilmot @BWPanda Hm, I fixed that late last night with the default file_display.json files on install. Maybe the sandbox needs a refresh...

there is no obvious way to add a video to a node, now that we allow uploading videos. Should there be an issue for that?

@docwilmot yes, probably. :)

But then the media types thing says not to use the same media type for more than one file type. So I guess that's not a valid use case...?

@BWPanda I do this all the time, I think that's bad advice. We should remove that text... where did you find it?

Would it be possible to grey out media types from the 'Available media types' fieldset that are already used in a file type? (since you can only use each media type once)

This is probably N/A since you can have as many file types per mime type as you want.

What's the 'Manage file display' section for, how does it differ from 'Manage displays', and should there be help text explaining as much?

This is the problem we are hoping to fix before release, this whole tab should be moved into a a "setting" for the file row on the manage fields tab, but that turns out to be hard.

How do you use files with content types? I can't see a way to choose a file type, and the File and Image fields look the same... The same with CKEditor.

@bwpanda It's exactly the same as it was before. The only difference is that now you can come to the manage files page, edit them, and add additional field data. We may look into adding something like inline entity form so that we can get those additional fields to show up in other places too. But, that's a follow-up. :)

@quicksketch
Copy link
Member

I tested this out further and it's working really well now. The most recent changes fixed the issue with the default file displays not being set. So now images display as <img> tags, videos as <video>, etc.

There's definitely a lot of room for improvement, the comments above all apply the File Entity module for Drupal 7 as well. But we need a starting point and this does a lot. And it has an upgrade path from D7. This is a great improvement to allow files to be uploaded directly and fieldable. Though we'll need a lot of follow-ups to get it integrated into all possible places.

Here are a few more items that could be follow-ups:

  • When looking at the list of files under /admin/content/files, there should be a "View" link under the drop button that takes you to the file page.
  • Similar to the above, the File::uri() function currently returns the path to the file. Perhaps this should be the path like /file/[fid], so it matches other types of entities? That'd be a substantial API change, maybe shelve that for 1.15 so we can discuss it.
  • The file admin listing shows a file usage count. That should be made into a link to the /file/[fid]/usage page now that we have that page.

I think this is ready to be committed as-is.

@quicksketch
Copy link
Member

Merged backdrop/backdrop#2838 into 1.x for 1.14.0. Wooohoooo!

Thank @Al-Rozhkov, @daggerhart, @docwilmot, @jenlampton, @olafgrabienski, @klonos, @herbdool, and @BWPanda! That was a huge effort and Backdrop is going to be substantially more awesome with this in. Now we have a basis for tons of the other work in the media management project board: https://github.com/backdrop/backdrop-issues/projects/6

@jenlampton
Copy link
Member

jenlampton commented Sep 4, 2019

Woohoo! I've added your comments to the list of things to do later in the top post, so they don't get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment