Currently, admin_title is being used as the entity label, but this is a bit strange since that field doesn't even appear to the user unless they make the FPP reusable. It should probably be the title field instead, which always appears for the user.

In addition, the title field should be required, as some tools, like the entityreference browser )http://drupal.org/sandbox/davidseth/1489176), expect entity_label to return something non-empty, and it probably should always be non-empty.

Patch forthcoming.

CommentFileSizeAuthor
#23 fieldable_panels_panes-n1588882-23.patch1.18 KBDamienMcKenna
#21 fieldable_panels_panes-n1588882-21.patch1.25 KBDamienMcKenna
#18 fieldable_panels_panes-entity_label-1588882-18.patch1.25 KBjlapp
#12 fieldable_panels_panes-entity_label-1588882-12.patch1.23 KBjlapp
#1 fieldable_panels_panes-title_label-1588882-1.patch740 bytesmcarbone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented
Status: Active » Needs review
FileSize
fieldable_panels_panes-title_label-1588882-1.patch740 bytes
merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

The admin title is specifically meant to possibly be different from the actual title of the content, because sometimes the visible title is different from how the content is identified. I am dubious about this change.

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented

Hm, what do you suggest as a solution then to something like entityreference_browser that requires a label? It seems to me that if Administrative title is meant to be the identifying title everywhere (and not just when a pane is reusable) then that should be the required field instead, and should be moved out of the reusable fieldset.

Stalski’s picture

Stalski CreditAttribution: Stalski commented

I am strongly in favor of how it is implemented right now. We had some different opinions internally about this ... but end conclusion is the way it is now.

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented

I am not quite sure what part you all are disagreeing with. The patch still allows a different title/admin title, it's just trying to resolve this issue:

entity_label... should always be non-empty.

Seems to me that at least one title field needs to be required -- whether it's title or admin_title I have no particular strong feeling toward.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I suppose if admin_title is empty for non-reusable panes, we could always copy the title into that field? That would solve the problem?

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs review » Needs work
mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented

Well, that doesn't the solve problem because title itself is currently not required. In fact, currently no fields are required -- a completely empty FPP can be created.

So perhaps another approach would be to:

a) Make Title required and use it for entity_label (which is what the patch in #1 does); and
b) Make Admin Title required if "Reusable" is checked

If there's an argument to be made for keeping Title non-required, I don't think I'm following it.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I often have needs for blocks with no titles.

mcarbone’s picture

mcarbone CreditAttribution: mcarbone commented

Sure, which is why the core block module requires "Block description", its analog to "Administrative title" here. So then is the solution to move that field up and always require it (reusable or not), and to keep it as the entity_label?

And then admin/structure/panels/entity/manage/[bundle] could be modified to use Admin Title as the primary column rather than Title.

caschbre’s picture

caschbre CreditAttribution: caschbre commented

We're looking at entity reference and FPP for an upcoming project and this poses a potential issue. Here's what we were considering as a solution.

1) If admin title is populated then we should be good to go.
2) If admin title is missing and title is populated then populate admin title with title.
3) If admin title and title are missing then populate admin title with a pattern.

A possible pattern could be: fpp__ (or something along those lines).

This should satisfy always having a value in admin title but a) not requiring the instance to be marked re-usable to populate admin title; and b) not requiring an entity title.

While it may be odd to see fpp__ in an entity reference field, the user still has the option to fill out the title or admin title.

jlapp’s picture

jlapp CreditAttribution: jlapp commented
Status: Needs work » Needs review
FileSize
fieldable_panels_panes-entity_label-1588882-12.patch1.23 KB

Building off of caschbre's suggestion, what about using a callback function for the entity label rather than Admin Title or Title?
The callback function could use the logic caschbre suggested in #11, where the label would be the Admin Title if it is populated, otherwise it would be the title if it is populated, otherwise a unique label would be generated based on a pattern (fpp__[bundle]_[fpid]).

This allows Title and Admin Title to remain not-required, allows them to have different values if the user specifies them, and makes sure there is always a non-empty label for Entity Reference and other modules to display. It should not add any DB performance overhead since the $entity object is already fully loaded in the label callback.

I'm attaching a patch that implements the solution I described. Any feedback on the approach or implementation is certainly welcome.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I like this patch very much.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I think this just needs a quick test from someone such as mcarbone to ensure it solves the original problem adequately and it can be committed.

caschbre’s picture

caschbre CreditAttribution: caschbre commented

Great work Jameson! I'll sync up with you Monday so we can tackle those other Panelizer and ERS issues.

eloivaque’s picture

eloivaque
Primary language Catalan
Location Catalan Countries
CreditAttribution: eloivaque commented

The patch of #12 it's works.

Status: Needs review » Needs work

The last submitted patch, fieldable_panels_panes-entity_label-1588882-12.patch, failed testing.

jlapp’s picture

jlapp CreditAttribution: jlapp commented
Status: Needs work » Needs review
FileSize
fieldable_panels_panes-entity_label-1588882-18.patch1.25 KB

Thanks for testing eloiv!
I see that the patch no longer applies cleanly to the 7.x-1.x-dev branch, so I've re-rolled it against the latest version. Hopefully this can be committed now.

nielsonm’s picture

nielsonm CreditAttribution: nielsonm commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Patch #18 works for me. Thanks!

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Parent issue: » #2244541: [meta] Plan for Fieldable Panels Pane 7.x-1.6 release
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
FileSize
fieldable_panels_panes-n1588882-21.patch1.25 KB

Patch reroll so it'll work once #1618308, #2237137 and #2237097 are committed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: fieldable_panels_panes-n1588882-21.patch, failed testing.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Needs work » Needs review
FileSize
fieldable_panels_panes-n1588882-23.patch1.18 KB

Rerolled.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Title: Use title for entity label and make required » Use title for entity label when admin_title isn't present
Status: Needs review » Reviewed & tested by the community

Patch works great in my testing!

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Reviewed & tested by the community » Fixed

Committed.

  • DamienMcKenna committed ac3c2e2 on 7.x-1.x 
    Issue #1588882 by jlapp, mcarbone, DamienMcKenna: Use title as the...

Status: Fixed » Closed (fixed)

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