Module asset provides function override asset in contextual menu for assets. This function allows user to change some fields only for this instance of asset. For example, he can change description for photo.

But on some projects I have asset with only one field - image. And this field cannot be overridden. When user tries to use "override" function, he sees empty form without fields and he can be at a loss because it's strange behavior.

empty form

I propose to add special option for asset type, to allow to disable overriding for exact type of asset.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eugene.ilyin created an issue. See original summary.

eugene.ilyin’s picture

Issue summary: View changes
FileSize
12.56 KB
eugene.ilyin’s picture

Status: Active » Needs review
FileSize
6 KB

I've prepared patch to solve this problem. I hope it would be useful.

IRuslan’s picture

Status: Needs review » Needs work

It's good catch to remove override if it's useless.
But i think better to pass some real asset options with _assets_get_overridable_fields() to JS instead of adding some option.
In this approach — if there is no fields to be overridden, override contextual option will be hidden.
So my idea is:
- during provisioning of field config on Drupal side check if there are field to override with _assets_get_overridable_fields()
- get this option in initialization of CKEditor menu and commands and do not init override

eugene.ilyin’s picture

It's good idea to use data from function _assets_get_overridable_fields().
But I still propose to give possibility to deny "override" option for some cases, even we have fields to override, because not all projects need this extra option and it can confuse content manager.

So I propose to hide option "override" if option "Deny to override fields for instance of asset" is active or we have no fields to override.

I've prepared new patch.

eugene.ilyin’s picture

Status: Needs work » Needs review
eugene.ilyin’s picture

I've updated patch because I found mistake in part for plugin.js

IRuslan’s picture

Hi, great work!

But i'm still thinking that we need to provide a generic way to configure contextual options in such case, instead of configuring each item separately.

About hiding override option automatically — it's good idea.
Just small detail. From what i see, editor.contextMenu.addListener callback called each time when we open menu.
Result of callback is the list of menu items for each certain situation (asset). This means that instead of deletion of previously added item (couple of lines above), we need to just do not add it.
So what we need to do is just to add new condition here:

if (conf && conf.modes && !(conf.modes.length === 1 && conf.modes.full && !conf.fields.length)) {

I propose to split your patch into 2 — fix with auto-hiding and configurable option — to make possible for others to re-use this feature if needed.

eugene.ilyin’s picture

IRuslan’s picture

IRuslan’s picture

From my point of view, the most acceptable solution is next.

We could add to place where we build context menu additional check, something like (it's rough example and should be well tested):

if (!element.data('asset-suppress-override')) {//...}
...
if (!element.data('asset-suppress-edit')) {//...}

It will allow:
1. Do not break any previous versions/setups. No any hook_updates needed.
2. Allow any custom developers in a flexible fashion to control the context menu even per certain asset instance if needed.
I.e. we could override theming of asset template per asset type and restrict it. Or to add this attribute to global asset template to restrict options at all. Or these attributes could be added on the fly via JS if needed.

In the future, we could improve this and make it dynamic, i.e.
<some-asset-tag-in-template data-asset-suppress-menus="assetcut,assetdelete,assetoverride, etc">

So in your case, you just need to modify asset template to add attribute where needed.

IRuslan’s picture

Status: Needs review » Needs work
eugene.ilyin’s picture

Done, could you check it please?

eugene.ilyin’s picture

Status: Needs work » Needs review

  • IRuslan committed ae50063 on 7.x-1.x authored by eugene.ilyin
    Issue #2559669 by eugene.ilyin, IRuslan: Allow do disable the "Override...
IRuslan’s picture

Status: Needs review » Fixed

Great work!
Committed to dev.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.