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.

Files: 
CommentFileSizeAuthor
#18 fieldable_panels_panes-entity_label-1588882-18.patch1.25 KBjlapp
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#12 fieldable_panels_panes-entity_label-1588882-12.patch1.23 KBjlapp
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldable_panels_panes-entity_label-1588882-12_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 fieldable_panels_panes-title_label-1588882-1.patch740 bytesmcarbone
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldable_panels_panes-title_label-1588882-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new740 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldable_panels_panes-title_label-1588882-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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.

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.

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?

Status:Needs review» Needs work

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.

I often have needs for blocks with no titles.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldable_panels_panes-entity_label-1588882-12_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

I like this patch very much.

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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.

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

Patch #18 works for me. Thanks!