Starting this as a support request.

If I have a global OG image tag outputting a default site image and then for my content types I have a more specific tag that uses the image in an image field.

If that image field has no image in it I get no OG image tag.

I think it would be useful in this case for the global image to override the empty value.

Now I understand maybe not everyone wants this behaviour but maybe it could be optional as I think a lot of people would want it for things like this image tag.

For a start, is this behaviour currently possible? It seems like it is not.

Files: 
CommentFileSizeAuthor
#8 metatag-parent_overrides-2044621-8.patch6.48 KBmrjmd
FAILED: [[SimpleTest]]: [MySQL] 100 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

DamienMcKenna’s picture

Version:7.x-1.0-beta7» 7.x-1.x-dev
Category:support» feature

This is one of the difficulties of using Token - the logic checks to see if fields are empty before applying the token rather than after. This is something I need to look into.

rooby’s picture

Thinking out load a little here.

At a quick look it seems like one problem is the metatag_config_load_with_defaults() function, which does a union of the config arrays, starting with the most specific and ending with the global.

This union means that you are basically going to get the entirety of the most specific config all the time because the config array is almost the same between instances. Values left empty in the more specific instance will override a non-empty value in the global one.

On top of that, what metatag_config_load_with_defaults() returns is a single array of raw values so if those values end up empty after token replacement you don't have the parent value available to use.

Seeing as the metadata config is cached, it could be feasible to load the parent configs into the cache as well as just the specific instance config and then use them after token replacement as required.

Either that or metatag_config_load_with_defaults() could return an array of config arrays, from specific to global, instead of just the most specific.

Then maybe before hook_metatag_metatags_view_alter() is invoked in metatag_metatags_view() there could be a check for empty values where it falls back to the parent and tries getElement() again.

Seems it might sometimes be desirable to have empty values though and not fall back to the parent. In which case maybe the global fields should be left empty? or else the alternative is to have a setting for each metatag to specify whether or not the parent can override an empty value.

rooby’s picture

Status:Active» Needs review
StatusFileSize
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

Here is a proof of concept that adds the ability to make each individual metatag allow overrides at the instance level.

If a metatag is set to allow overrides and is left empty or token replacements render it empty, then it will check the parent instance for a metatag.

If the parent is also empty and also allows overrides the process continues.

It could probably do with a bit of refactoring and it doesn't take into acount the fact that the allow override setting is useless on the global instance, but it shows a possible solution.

What do you think?

rooby’s picture

Issue summary:View changes

Adding extra info.

rooby’s picture

Issue summary:View changes
StatusFileSize
new6.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch metatag-parent_overrides-2044621-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This version applies to latest dev and fixes a couple of warning messages that were coming from the code in the old version.

Status:Needs review» Needs work

The last submitted patch, 4: metatag-parent_overrides-2044621-4.patch, failed testing.

jeffdiecks’s picture

Issue tags:+Needs reroll
mrjmd’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll+SprintWeekend2015
StatusFileSize
new6.48 KB
FAILED: [[SimpleTest]]: [MySQL] 100 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reroll attached.

Status:Needs review» Needs work

The last submitted patch, 8: metatag-parent_overrides-2044621-8.patch, failed testing.

tomogden’s picture

I think an "override" is not strong enough. If a saved default field is emptied, one would expect it should no longer have any effect.

From what I can see, Metatag already does the following:
1) If a node's metatag field is not filled in, that tag is not saved, and the tag inherits from the parent defaults. I think this is already the case.
2) If a saved default field is not filled in, that tag is not saved, and the tag inherits from the parent default.

Thus, it should logically follow (but does not):
3) If a previously saved field is emptied, that tag should be dropped from the default record, and it should revert back to the above behavior.

To me, not following Rule 3 is a bug.

Seems it might sometimes be desirable to have empty values though and not fall back to the parent. In which case maybe the global fields should be left empty? or else the alternative is to have a setting for each metatag to specify whether or not the parent can override an empty value.

While it may be desirable to have defaults overriden by a NULL value, this should be the exception, not the rule. Thus, if there IS an override control, it should be to override inherited text with a NULL value. But my guess is it's not too common to want to empty or reduce your metatags. Usually people go overboard in the other direction.

tomogden’s picture

Title:Allow parent config to override empty values of more specific config» Metatag forms save empty field values which block default inheritance.
Category:Feature request» Bug report
Status:Needs work» Needs review
StatusFileSize
new2.39 KB
PASSED: [[SimpleTest]]: [MySQL] 101 pass(es).
[ View ]

I am submitting a patch that does the following two things:

  1. Prevents Metadata from saving empty fields.
  2. Prevents empty field values previously saved in the database from blocking parent configurations.

While this meets the functional requirements to prevent the unexpected behavior, it does not improve the UX, which can still confuse users. For that, I would propose THREE new things:

  1. Adding an [empty] token (or something like it).
  2. Filling the fields' 'default_value' with that configuration's saved values (imagine that), instead of the aggregated parent values.
  3. Placing the aggregated parent values into the fields' 'placeholder' so that they display in gray and are replaced when clicking into the field.

These should remove all the ambiguity currently confusing users. I think I also see a number of related issues posted which may be likewise resolved.

Then we can talk about deprecating the shell game operation in Metadata where the aggregated parent values are deleted from each field just before they are saved.

rooby’s picture

Can you clarify whether your patch is intended to replace the original patch here or if it is to go with it?

If the former then it should go in a new issue because that is not what this issue is actually about.

If the latter I will try to find some time to review it and roll it all into a single patch.

rooby’s picture

Oh I just also noticed your change of issue title.

I will try your patch and see if it fixes my original issue but I think yours is a different issue.

DamienMcKenna’s picture

Status:Needs review» Closed (won't fix)

Here's a problem - how is Metatag supposed to know if you are trying to reset a value to its default or you just don't want to output a specific meta tag?

I'm closing this in favor of #1798984: No easy/clear way to revert meta tags on an object to their defaults.

rooby’s picture

Title:Metatag forms save empty field values which block default inheritance. » Allow parent config to override empty values of more specific config
Status:Closed (won't fix)» Needs review

My original intention for this issue got a little hijacked.
Since I got no reply from my last question I'm reverting the title.

My proposal was that if there is something entered into a child metatag field and then after token replacement that value is empty (no field value), then if the override option is selected for that specific meta tag it will try using the parent one.

My interface put a checkbox for every metatag so you could specifically enable the parent override at the tag level. Admittedly the UI could use a little love but I didn't want to spend too much time yet in case there's no chance of it being committed. I could make it nicer.

I most commonly use this patch for images so that I can set facebook og images to an image field and default to a generic site image if there is none for the given node.
Seems to work fine.

#8 is the last iteration of this patch. I will test shortly to see if it needs a reroll but if you still want to close this feel free, although I think it will be hard for me to write an add-on module to achieve this.

DamienMcKenna’s picture

Category:Bug report» Feature request
Status:Needs review» Needs work

This would be a definite change in the current architecture so would require some sort of backwards compatibility to not skew existing sites.

rooby’s picture

Well the default settings result in the status quo. It's only once you start checking the check boxes that functionality starts to differ, so this should not affect existing sites.

DamienMcKenna’s picture

rooby’s picture

Status:Needs work» Needs review

Setting back to needs review based on #17.

rooby’s picture

Status:Needs review» Needs work

Actually my initial patch needs a reroll.