Problem

Field collection's markup is not accessible to themers. Most of it is added in the formatters, with markup, some classes, and the operation links inserted directly into prefix and suffix, making it very difficult to alter. This is a problem for any project using modern front end practices (like OOCSS, SMACSS, and mobile first development) that needs to control class names, html structure, and minimize markup to improve performance.

Proposed resolution

All of field collection's markup should be easily editable in a tpl. Classes and attributes should be alterable through normal Drupal preprocessing, and the tpl should have useful theme hook suggestions. All formatters provided by the module should use the same tpl, and give pro themers the same render array properties to play with. The default theme css should be simple and easy to extend and override.

Remaining tasks

Review the patch, please :D.

User interface changes

Revises and streamlines the formatter choices and adds states to the formatter for improved ux.

API changes

Puts all of the module theme output, including operation links, in a single tpl used by all formatters. Simplifies theme css, removing all floats. Adds small render arrays for the operation links. Removes the "Fields Only" formatter and replaces it with a checkbox in the "Field Collection Items" formatter. Adds custom classes and theme hook suggestions in a preprocessor, available for addition and alteration in the normal theming fashion. Normalizes the render array and view mode handling between formatters.

Original report by RobW

Right now the wrapping code for field collections, along with the add/edit/remove list and everything available in the manage display tab is provided directly by the module, and collapsed into #prefix and #suffix. It would be fantastic if the wrapping code was available in a tpl, and if the various components (wrapper, add/edit links, label) were separate variables.

This would bring field collection closer to the theming structure of good old D6 multigroup.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Makes sense, patch welcome!

tim.plunkett’s picture

dman’s picture

Yeah. Good request.

RobW’s picture

Assigned: Unassigned » RobW

OK, I'm on this in my free time in the next week or two.

Any other issues besides #1276258: Completely hide empty field collections that I should look over while I'm working on this?

RobW’s picture

OK, question: would it be possible to use the hidden widget a.k.a. front end operation links and the embedded widget at the same time?

As long as I'm pulling all markup into the theming function, it makes sense to make an operation links tpl too. Which made me wonder if the operation links need to be shown and hidden based on the widget, or if it would be useful to make this a separate choice.

[edit] Looks like they're not connected. Been a while since I used FC.

dman’s picture

As much as I love the operation links (and I've been able to theme them quite usefully) I'm wondering it we should look at leveraging 'contextual' in-place edit links instead or as an option.
Been doing a lot of in-place edit and AJAX stuff based on and working around or with the field-collection buttons.
Now I've got a handful of add-on modules that each add various widget extensions in different ways. It seems that in D7 maybe embracing contextual a little more consistently *MAY* be a direction to look.
Just todays thoughts.

RobW’s picture

I think the operation links are a different use case than contextual links. Whether it was meant this way or not, the operation links are really useful for building a simple, themed ui for end users with edit privileges, but who don't have privileges to use any module with contextual links. Maybe it's an odd use case, but the code is there and I have used them this way, and wouldn't want to replace it with a ui familiar to Drupal admins only. But a helper module that lets you add/edit field collections with contextual links could be cool.

RobW’s picture

Status: Needs work » Active

My plan for field collection tpls, here mostly for my own reference:

field--field-collection.tpl.php, the field tpl extended?
-- add op links and description.

field-collection-item.tpl.php
-- content (render fields)
-- op links: edit, delete (rendered)

field-collection-item-link.tpl.php, for the "list/links" formatter
-- content (print links formatted by formatter)
-- op links: edit, delete (rendered)

field-collection-op-links.tpl.php
-- op links and description (vars)

Also reduce formatters to "Links" and "Field Collection Items", with a checkbox for Operation links, instead of a third formatter "Fields Only". After the patch, everything that's different between "Field Collection Items" and "Fields Only" (besides the op links) should be easy to change in the tpls.

RobW’s picture

Status: Active » Needs work
FileSize
14.78 KB

Running into a rendering problem that seems simple but is driving me cray cray. I abstracted the field collection operation links into a function that attaches a render array to the field collection field and the field collection items. It renders fine on the field collection items, and anywhere else I've attached it to debug, but it doesn't render on the field collection field.

I have a suspicion it has something to do with theme_field, which the field collection field render array is rendered by. I attached my array as a child of an array rendered by theme_entity, and that worked fine. Or, maybe I am going about attaching a render array as a child of another in the wrong way, who knows? Any suggestions are welcome.

(Haven't written any preprocess functions for the op links tpl yet, so it's ugly. Feel free to pretend that doesn't exist.)

RobW’s picture

Status: Active » Needs work

Decided to just drop the rendered op links in #suffix. The op links tpl is still editable, but total render array tomfoolery is only available for the edit links. I may revisit when everything else is working well.

RobW’s picture

Title: Add a field-collection.tpl » Move markup to template files and improve theming

OK, almost done with this. One question I want to throw out to everyone is about template suggestions.

Field has its normal template suggestions, no surprises there.

Field Collection Item:
Here I'd like to work with view mode (e.g., teaser), field collection name, and parent entity bundle (e.g., page, article). The cascade gets pretty long, but I'm ok with 7 if everyone else is.

base template: field-collection-item.tpl.php

  1. field-collection-item--[view mode].tpl.php
  2. field-collection-item--[parent entity bundle].tpl.php
  3. field-collection-item--[field collection name].tpl.php
  4. field-collection-item--[parent entity bundle]--[view mode].tpl.php
  5. field-collection-item--[field collection name]--[view mode].tpl.php
  6. field-collection-item--[field collection name]--[parent entity bundle].tpl.php
  7. field-collection-item--[field collection name]--[parent entity bundle]--[view mode].tpl.php

Field Collection Operation Links:
These are the add, edit, and delete links. Op type is either 'add' or 'edit-delete'.

base template: field-collection-op-links.tpl.php

  1. field_collection_op_links--[op type].tpl.php
  2. field_collection_op_links--[view mode].tpl.php
  3. field_collection_op_links--[field collection name].tpl.php
  4. field_collection_op_links--[op type]--[view mode].tpl.php
  5. field_collection_op_links--[field collection name]--[op type].tpl.php
  6. field_collection_op_links--[field collection name]--[view mode].tpl.php
  7. field_collection_op_links--[field collection name]--[op type]--[view mode].tpl.php

If anyone has opinions, let me know.

RobW’s picture

Here's an incomplete patch for testing and suggestions.

Done right now:

  • Removed markup in module, moved to tpl files (except links to collection formatter). As a side effect, this fixes #1276258: Completely hide empty field collections.
  • Abstracted op link code to a helper function.
  • Created a field-collection-op-links.tpl.php.
  • Revised field collection items formatter. Combined "field collection items" and "fields only" by adding a formatter setting to include or disable op links.
  • Removed field_collection_view theme wrapper. Added its classes and attributes to field-collection-item.tpl.php, along with some other revisions.
  • Right now this requires and includes the changes made in #1688114: Clean up field_collection_item_access() and the calls to it..

Left to do:

RobW’s picture

[Edit] Nevermind.

While thinking about how to do the links formatter tpl, I started to wonder why we're theming field collection items individually instead of with a foreach. Structure would look like:

field.tpl

 field-collection.tpl
 - (foreach items as item)
 -- wrap div
 --- render field collection item
 -- end wrap div
 
 -- edit link ul
 - (end foreach)

 - add link ul
 end field-collection.tpl

end field.tpl

Using a foreach would solve some problems and simplify a lot of things:

  1. User would be able to add wrapper divs for the whole field collection, do all entity_metadata_wrapper() theming, and whatever else field-collection-specific-but-outside-of-the-items theming in field-collection.tpl instead of field--field-collection.tpl.
  2. Could remove the field-collection-op-links.tpl, at the expense of having to theme op links individually for each formatter with its own theme output. Could get rid of that long list of op links template suggestions, too.
  3. Wouldn't need the #suffix hack for add links I mentioned in #9.

Downsides:

  1. field-collection-item.tpl would be removed, breaking current themes that use the dev version in the wild. Would require a new major version probably.
  2. More work on the patch :).
  3. ...

Am I missing an obvious downside? If anyone has thoughts let me know; I'm going to start refactoring today.

RobW’s picture

That obvious downside I was missing is that field.tpl handles wrapping multiple values already. I strip out so much wrapper code I forget that sometimes. A unified field.tpl trumps the benefits of everything above.

nairb’s picture

The field.tpl wrappers are handy. I just used them to create a list of my collection items. I am working on migrating a big d6 site and I thought I would need to mess with this templating more. In the end all I had to do was override field-collection-item.tpl.php to clean up some of my output.

I can see templates for view mode, field collection name, and parent entity bundle being useful. Especially when displaying content in various context.

It's looking good

RobW’s picture

Some more notes:

The add link presents a bunch of problems. There are three ways we can attach it to the field collection:

  1. Insert the add link in the field #suffix. This is the pre-patch placement.
  2. Result:

    field wrap (field.tpl)
    - field item (field.tpl)
    -- field collection item (field-collection-item.tpl)
    - end field item
    - field item (field.tpl)
    -- field collection item (field-collection-item.tpl)
    - end field item
    end field wrap
    ADD LINK

    Dealbreaker: the add link is inserted after and outside of all field markup, breaking document structure and css theming.
     

  3. Insert the add link after the last field-collection-item with a tpl or #suffix.
    Result:
    field wrap (field.tpl)
    - field item (field.tpl)
    -- field collection item (field-collection-item.tpl)
    - end field item
    - field item (field.tpl)
    -- field collection item (field-collection-item.tpl)
    --- collected field
    --- collected field
    -- end field collection item
    -- ADD LINK
    - end field item
    end field wrap
    

    Cons: The add link is inside of the last field item. The ideal placement would be outside of the last field item, but inside the field wrap.
     

  4. Create field__field_collection theme function or template, and insert the add link in a custom variable.
    Result:
    field wrap (field.tpl)
    - field item wrap (field.tpl)
    -- field collection item (field-collection-item.tpl)
    - end field item wrap
    - field item wrap (field.tpl)
    -- field collection item (field-collection-item.tpl)
    - end field item wrap
    - ADD LINK
    end field wrap
    

    Perfect placement! But, Dealbreaker: modules and themes that provide their own theme_field implementation (e.g. base themes) will have to override both theme_field and theme_field__field_collection.
     

Although 3 gives us better placement it makes field collection more of a pain to theme overall. I'm going to use option 2 in the patch.

RobW’s picture

Here's an updated patch for anyone who's interested in progress on this issue. The new stuff:

  • Decided to abandon the separate op links formatter and field-collection-op-links.tpl. The op links are now provided as variables in field-collection-item.tpl. It adds more stuff to the field collection item template, but overall I think it makes theming and code maintenance easier.
  • Refactored field_collection_field_formatter_links, streamlined a ton of code and made the links available through the same template file as the rendered items. Question: I added a "field-collection-item-link" class on each field collection item output as a link. Do people like that or would they prefer a "field-collection-item-links" class on the field collection field itself? And do we want a separate tpl or template suggestion for field collection items output by the links formatter?
  • Improved formatter settings and summary on the manage field display page.

Note: this patch applies to the 7.x-1.x-dev from around July 26. There have been a bunch of commits since then that are going to necessitate a manual re-roll and maybe some code changes to take advantage of new features. Only that and css stuff left, almost ready for serious review!

RobW’s picture

Here we go. Attached is a complete patch, rolled against the latest dev. It requires you to apply the patch in http://drupal.org/node/1688114#comment-6310850 first, which although it keeps failing the simpletest is working fine for me.

The final rundown of changes:

  • Moves all markup to field-collection-item.tpl.php.
  • Adds template variables for operation links and field help text.
  • Revises formatters. Removes "fields only", and replaces it with an option in the "field collection items" formatter that does the same thing.
  • Revises css, removing all floats and sticking closer to the theme's default field styles. Class based, with less dependency on html structure. (I used Drupal's verbose css naming conventions, as much as it pained me. But if you don't like them, they're easy to change in the tpl and through normal preprocessing of classes_array now.)
  • Fixed issues with css attachment causing empty field collections to display the field collection label.
RobW’s picture

FileSize
13.7 KB
191.64 KB
31.61 KB

Screenshots of the new formatter options, and the css with the items and list formatters:

formatters
items
list

RobW’s picture

And here's what the template file looks like:

<?php
/**
 * @file
 * Default theme implementation for field collection items.
 *
 * Available variables:
 * - $content: An array of collected fields. Output all fields with
 *   render($content). Use hide($content['field_example']) to hide a field
 *   before printing render($content), and render($content['field_example'])
 *   to output a single field.
 * - $title: The (sanitized) field collection item label.
 * - $url: The direct url of the current entity.
 * - $page: Flag for the full page state.
 * - $add, $edit, $delete: The title of the operation link if available,
 *   or FALSE.
 * - $add_path, $edit_path, $delete_path: The path to the operation.
 * - $help_text: The field collection field help text, normally displayed on
 *   the field collection edit form.
 * - $classes: String of classes that can be used to style contextually through
 *   CSS. It can be manipulated through the variable $classes_array from
 *   preprocess functions. By default the following classes are available, where
 *   the parts enclosed by {} are replaced by the appropriate values:
 *   - field-collection-item
 *   - view-mode-{view_mode}

 * Other variables:
 * - $classes_array: Array of html class attribute values. It is flattened
 *   into a string within the variable $classes.
 * - $content_attributes: HTML attributes added by modules (e.g., RDFa).
 * - $view_mode: View mode, (e.g., 'full', 'teaser').

 * Template suggestions:
 * - field-collection-item--[view mode].tpl.php
 * - field-collection-item--[parent entity bundle].tpl.php
 * - field-collection-item--[field collection name].tpl.php
 * - field-collection-item--[parent entity bundle]--[view mode].tpl.php
 * - field-collection-item--[field collection name]--[view mode].tpl.php
 * - field-collection-item--[field collection name]--
 *   [parent entity bundle].tpl.php
 * - field-collection-item--[field collection name]--
 *   [parent entity bundle]--[view mode].tpl.php
 *
 * @see template_preprocess()
 * @see template_preprocess_entity()
 * @see field_collection_preprocess_entity()
 * @see template_process()
 */
?>

<?php if ($content): ?>
  <div class="<?php print $classes; ?>" <?php print $attributes; ?>>

    <div class="content"<?php print $content_attributes; ?>>
      <?php
        print render($content);
      ?>
    </div>

    <?php if ($edit || $delete): ?>
      <div class="field-collection-op-links">
        <?php if ($edit): ?>
          <a class="field-collection-op-link field-collection-edit-link" href="<?php print $edit_path; ?>"><?php print $edit; ?></a>
        <?php endif; ?>

        <?php if ($delete): ?>
          <a class="field-collection-op-link field-collection-delete-link" href="<?php print $delete_path; ?>"><?php print $delete; ?></a>
        <?php endif; ?>
      </div>
    <?php endif; ?>

  </div>
<?php endif; ?>

<?php if ($add): ?>
  <div class="field-collection-op-links field-collection-op-links-add">
    <?php if ($help_text): ?>
      <p class="description"><?php print $help_text; ?></p>
    <?php endif; ?>
    <a class="field-collection-op-link field-collection-add-link" href="<?php print $add_path; ?>"><?php print $add; ?></a>
  </div>
<?php endif; ?>

It's longer, but I think a good tradeoff for having complete control of the operation links and content in a single file. If a developer doesn't ever want to use the op links, they can shorten this by half.

RobW’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field-collection-add-tpls-1157794-18.patch, failed testing.

RobW’s picture

Status: Needs work » Needs review
FileSize
32.64 KB

Ah, of course it failed since the patch required another...

Here's one with the changes in #1688114: Clean up field_collection_item_access() and the calls to it. rolled in.

Status: Needs review » Needs work

The last submitted patch, field-collection-add-tpls-1157794-22.patch, failed testing.

dman’s picture

bravo!

I'll have to try it out..

RobW’s picture

I made friends with simpletest. All should be good now.

Attached are two versions of the patch. The first is just the the changes for this issue, and requires that you apply the patch in http://drupal.org/node/1688114#comment-6311534 first. The second is both issues combined so you can apply it and go.

dman’s picture

Patch 2 in #26 seemed to create the field-collection-tpl.php file in the module directory instead of the theme/ directory for me, but once I moved that there, it started behaving as expected.
So far pretty good.
Now to remember what it was that I wanted to theme about it - like getting those button/links to behave consistently with the rest of the UI...

paulpaulti2’s picture

Just a side note:

If you already created a field collection and then applied the patch you have to edit the field_collection in the backend, otherwise the output of this collection in the frontend is zero, that took me some time to recognize :)

paulpaulti2’s picture

Category: feature » bug

Hi,

i experinece the following message if i edit content:

Notice: Undefined index: de in field_collection_field_attach_form() (Zeile 1262 von /var/customers/beigang/urbane-gaerten/contents/sites/all/modules/field_collection/field_collection.module).

Does this concerns my specific content type or do you also get the message?

Edit:
This message only appears when "Hide blank items" in field management for the field_collection is checked.
If i untick the option the message does not appear.
See the following screenshot: http://www.icepic.de/show/hide-blank.png/

Edit 2:
Is there any opinion against integrating zebra/even-odd function ( http://drupal.org/node/1378724 ) here?

Edit 3:
The patch http://drupal.org/node/1378724 also provide functionality to output field collection "id", which sometimes can be really usefull in theming purpose, why not integrate into your new version and in the example template "field-collection-item.tpl.php", it is working like

<div class="<?php print $zebra . ' ' . $id; ?>">
<?php print render($content); ?>
</div>

Regards,
paul

RobW’s picture

Thanks for the thorough review, Paul.

Needing to re-save makes sense, since two of the formatters have been renamed or removed. I'll take a look at fixes for that.

I haven't seen a "de" error. I assume de is referring to Deutch, so it's probably a language issue. Do you get it the same error if just this patch is applied?

As for zebra striping and id's, both should be easy to impliment yourself in the tpl file. Zebra is already handled by the field tpl, so I would not want to repeat it here. Depending on your browser requirements you can avoid using a class altogether with the CSS selector :nth-child(odd) or :nth-of-type(odd).

For ids, I took a look at the patch you linked and I don't really like the method it uses. I think a simpler and more robust way to add ids would be to use custom theming and the entity id, which although isn't listed in the tpl file comments I believe is available as $id.

I think both of these are great examples of project specific requests that we wouldn't want to add to the ui, but would be easy for a front end developer to add with a well documented tpl. I'll add some comments about $zebra and $id in the next patch roll.

paulpaulti2’s picture

Hey Rob,

what do you mean with "Do you get it the same error if just this patch is applied?" ? As you already said, "de" should be the standard language key from my drupal installation.

I would love to use CSS pseudo selectors like ":nth-child", but for Microsoft Browsers only since IE9 we have support so ... most of the time it is isnt the way i can go :(

"I think both of these are great examples of project specific requests that we wouldn't want to add to the ui, but would be easy for a front end developer to add with a well documented tpl. I'll add some comments about $zebra and $id in the next patch roll."

That is perfect, no need of zebra/id in UI but documented in template is great!

Thanks for your effort.

RobW’s picture

Revised tpl documentation to include zebra and id.

Status: Needs review » Needs work

The last submitted patch, field-collection-add-tpls-all-in-one-1157794-32.patch, failed testing.

RobW’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field-collection-add-tpls-all-in-one-1157794-32.patch, failed testing.

RobW’s picture

Status: Needs work » Needs review

Eh, well it passes simpletest on my local machine. I think the testbot is just having some issues.

The last submitted patch, field-collection-add-tpls-all-in-one-1157794-32.patch, failed testing.

dman’s picture

Status: Needs review » Needs work

It's complaining about the patch format itself, not the tests.

Detect a non-applicable patch
Ensure the patch applies to the tip of the chosen code-base.
Command [git apply --check -p1 /var/lib/drupaltestbot/sites/default/files/review/field-collection-add-tpls-all-in-one-1157794-32.patch 2>&1] failed
  Duration 0 seconds
  Directory [/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/field_collection]
  Status [1]
 Output: [error: patch failed: field_collection.module:592
error: field_collection.module: patch does not apply].

Though I can't visually see what it's actually complaining about. Maybe it needs a re-roll against -dev?

RobW’s picture

Ah, that's a new message. Before it was giving me "could not initialize mysql environment" or something similar.

RobW’s picture

Rebased. Let's try this one.

Sorry for all of the junk comments.

RobW’s picture

Status: Needs work » Needs review
RobW’s picture

Issue summary: View changes

Updated the issue summary with all changes up to comment #40

RobW’s picture

jisuo’s picture

Hey RobW!

How complete is this patch? I would like to review it (and with review I mean try and see if it works, not sure I would find anything by looking at it line by line). Which version of the module should I use? How do I apply this patch without manually copying code from the patch source?

edit: found this on the last question http://drupal.org/patch/apply so got that covered.

RobW’s picture

The patch is complete, just looking for bugs and the maintainers to chime in and give their feedback. You can apply it to the latest 7.x-1.x-dev version.

tim.plunkett’s picture

Assigned: RobW » tim.plunkett

Putting back on my radar.
RobW, thank you for all of your work here!

dman’s picture

Assigned: tim.plunkett » RobW

I've been running it on a pre-live site for a while with no bad effects, though I still haven't pushed my themer to test it out properly yet.
Status = not broken, and it works.

tim.plunkett’s picture

Assigned: RobW » fago
Status: Needs review » Needs work

So, thi

+++ b/field_collection.moduleundefined
@@ -751,20 +773,23 @@ function field_collection_item_is_empty(FieldCollectionItemEntity $item) {
 function field_collection_field_formatter_info() {
   return array(
-    'field_collection_list' => array(
-      'label' => t('Links to field collection items'),
+    'field_collection_items' => array(
+      'label' => t('Field collection items'),
       'field types' => array('field_collection'),
       'settings' =>  array(
+        'show_op_links' => FALSE,
         'edit' => t('Edit'),
         'delete' => t('Delete'),
         'add' => t('Add'),
         'description' => TRUE,
+        'view_mode' => 'full',
       ),
     ),
-    'field_collection_view' => array(
-      'label' => t('Field collection items'),
+    'field_collection_links' => array(
+      'label' => t('Links to field collection items'),
       'field types' => array('field_collection'),
       'settings' =>  array(
+        'show_op_links' => FALSE,
         'edit' => t('Edit'),
         'delete' => t('Delete'),
         'add' => t('Add'),
@@ -772,13 +797,6 @@ function field_collection_field_formatter_info() {

@@ -772,13 +797,6 @@ function field_collection_field_formatter_info() {
         'view_mode' => 'full',
       ),
     ),
-    'field_collection_fields' => array(
-      'label' => t('Fields only'),
-      'field types' => array('field_collection'),
-      'settings' =>  array(
-        'view_mode' => 'full',
-      ),
-    ),

This means we can't put this into 7.x-1.x. It'd have to be 7.x-2.x. Assigning to fago for his opinion.

RobW’s picture

I was thinking that would be the case. Is there a Drupal way to write a script to update a user's settings?

tim.plunkett’s picture

RobW’s picture

Thanks, Tim. So it looks like you can't do schema updates inside of a major module version, which I guess makes sense.

tim.plunkett’s picture

You can 100% do them, but its too fundamental of a change to pull on everyone, I think.
Are you ever on IRC? Maybe it'd be easier to hash this out in real time.

RobW’s picture

I am. Send me a PM -- I could schedule some time this Fri, Sat, or late Sunday.

jisuo’s picture

RobW: I tried it out. Created several field collections added different tpl-files and everything seemed to work as intended. Good work!

gillarf’s picture

Great work RobW.

Possible stupid question: will this work with field collection table (http://drupal.org/project/field_collection_table), or does it replace it?

i.e. should i now format my data in a table in the appropriate tpl.php file?

i tried it with the field collection table, and there was an error

PHP Fatal error: Call to undefined function field_collection_field_formatter_links() in .../sites/all/modules/field_collection_table/field_collection_table.module on line 122

RobW’s picture

Yeah, I revised and simplified all the formatters and the op links in this patch so it will probably break fc formatter add-on modules that relied on specific code in the main module. Another reason these changes might have to go into a 2.x branch.

You can theme your own table, but you'll have to do it with both field.tpl (targeting your field collection field) and the new field-collection-items.tpl (targeting just the fc items that are in your field). Fc table sort of does the same thing -- it takes all of the fc items which would normally each be in their own field item and returns the whole table as a single field item.

After the patch is finalized with input from the maintainers, I would be open to patching the most used 3rd party formatters (table and ...?). Thanks for bringing fc table up -- it's the first time I've looked at the code and I see some things I could do with this patch that might make third party formatter integration easier.

fago’s picture

Assigned: fago » Unassigned

Well, field collection is still in beta so I don't think we should worry *too* much about changing things. It still needs to evolve. That said, I think we should make sure that people settings survive and just change it - the chance is probably not big enough to justify rolling a 2.x branch.

@renamed-formatters:
The new naming is better, but at least we need to make sure to update settings. The HTML stays the way it is as well, right? We should ensure that.

- *   The operation being performed. One of 'view', 'update', 'create', 'delete'.
+ *   The operation being performed. One of 'view', 'edit' or 'update', 
+ *   'add' or 'create', or 'delete'.

We should keep sanity in the possible operations and either go with 'edit' or 'update', 'add' or 'create' but not both.

RobW’s picture

Thanks for taking a look, Fago. Responded to your questions on the access checks in http://drupal.org/node/1688114#comment-6411696.

IIRC, taking markup out of #prefix and #suffix changed some markup structure, and I definitely revised and added some classes. I completely rewrote the css as well, simplifying and removing all floats and clearfixes. I'll put up an html output comparison tonight so we can talk about what/why things changed.

gillarf’s picture

Thanks for the advice on using field.tpl as well as your new templates. I have managed to present data in tables now without empty field collections (hurray!).

Patching the field collection table formatter would be very much appreciated (i would help but its beyond me at the moment!)

RobW’s picture

Here's a comparison of output before and after the patch, with notes.

Current Markup:

// field.tpl code
<div class="field field-name-field-test-collection field-type-field-collection field-label-above">
  <div class="field-label">Test Collection:</div>
  <div class="field-items">
    <div class="field-item even">
// end field.tpl code  

// field collection code
      <div class="field-collection-view clearfix view-mode-full">
        <div class="entity entity-field-collection-item field-collection-item-field-test-collection clearfix" about="/field-collection/field-test-collection/1" typeof="">
          <div class="content">
          // Collected fields output here
          </div>
        </div>

        <ul class="field-collection-view-links">
          <li class="edit first"><a href="/field-collection/field-test-collection/1/edit?destination=node/1">Eduit</a></li>
          <li class="delete last"><a href="/field-collection/field-test-collection/1/delete?destination=node/1">Delete</a></li>
        </ul>

      </div>
// end field collection item code

// field.tpl code
    </div>
  </div>
</div>
// end field.tpl code

// field collection markup added outside of the field template
<div class="description field-collection-description">Help Text</div>

<ul class="action-links action-links-field-collection-add">
  <li><a href=  "/field-collection/field-test-collection/add/node/1?destination=node/1">Add</a></li>
</ul>
// end field collection code
  • The second level div wrapper contains the default entity classes (which mostly repeat the existing field classes) and a clearfix.
  • The add link is inserted in the field suffix, outside of all field markup. It can't be easily themed per field, isn't contained by field layout, and has no semantic relationship to field or field collection.

New Markup:

// field.tpl code
<div class="field field-name-field-test-collection field-type-field-collection field-label-above">
  <div class="field-label">Test Collection:</div>
  <div class="field-items">
    <div class="field-item even">
// end field.tpl code  

// field collection code
      <div class="field-collection-item view-mode-full"  about="/field-collection/field-test-collection/1" typeof="">
        <div class="content">
          // Collected fields output here
        </div>

        <div class="field-collection-op-links">
          <a class="field-collection-op-link field-collection-edit-link" href="/field-collection/field-test-collection/1/edit?destination=node/1">Eduit</a>
          <a class="field-collection-op-link field-collection-delete-link" href="/field-collection/field-test-collection/1/delete?destination=node/1">Delete</a>
        </div>
      </div>
      
      <div class="field-collection-op-links field-collection-op-links-add">
        <p class="description">Help Text</p>
        <a class="field-collection-op-link field-collection-add-link" href="/field-collection/field-test-collection/add/node/1?destination=node/1">Add</a>
      </div>
// end field collection item code

// field.tpl code
    </div>  
  </div>
</div>
// end field.tpl code  
  • Removed all clearfixes (and floats in the css).
  • Changed classes to be specific to field collection, and added a view mode class that follows patterns that exist in other modules. Everything can still be styled with the same specificity using the field classes.
  • Moved the field help text and "add" op link inside the field, but outside of the field-collection-item markup. See reasoning at comment http://drupal.org/comment/reply/1157794#comment-6287596.
  • Added classes in the standard verbose Drupal fashion for op links. Wouldn't be my choice, but with the markup in a tpl it's easy to change.
jnettik’s picture

I just used the patch in #40 against the dev version and it seems to be working perfectly for me. Thanks RobW for all the work you put into on this.

fago’s picture

I see, thanks for the comparison. In that case I'd say the probability for breaking people's site is quite high, so let's better roll it into a 2.x branch such that people have to manually trigger the update.

Let's implement the hook_update() though (as discussed), so that there is an upgrade path from the old formatters.

junedkazi’s picture

If I set the Add text to empty string I get the following error.

Undefined index: add in field_collection_field_formatter_settings_summary() due to array_filter used to get the links array.

So just added the isset check and changed the inline condition check format.

junedkazi’s picture

Status: Needs work » Needs review

The test is postponed due to #1782664: Call to undefined function entity_revision_is_default() . But it looks like this issue is fixed. Do I have to change the dependencies in the info file to check against the dev version of entity api ??

marcusx’s picture

Status: Needs review » Needs work
marcusx’s picture

Status: Needs work » Needs review

EDIT:

Mmmm - how can I reactivate a "Postponed" test?

junedkazi’s picture

Status: Needs review » Needs work
junedkazi’s picture

Status: Needs work » Needs review

Test back in queue.

marcusx’s picture

Status: Needs review » Needs work
FileSize
28.78 KB

Mmm not working. Still postponed. Uploading patch again with new name.

marcusx’s picture

Status: Needs work » Needs review
BBC’s picture

Many thanks for the work that's gone into this so far.

I've been experimenting this patch (#68) and am finding that the edit and delete links are not rendered. The add link appears when a field collection has no data, but when multiple values are allowed, add disappears as well.

Any suggestions as to what I might be missing? I'm using the latest dev version and the field template from #20.

RobW’s picture

Did you configure the operation links in your content type's manage display? You need to add the link titles there in order to have them show up.

The add link shouldn't render at all when the show op links setting isn't set, and should always show when it is. The offending code is probably:

if (!$items) {
  // If a field collection has no items, attach the add link if available.
  field_collection_field_formatter_op_links($element, $entity_type, $entity, $field, $instance, $langcode, $items, $display);
}

I don't remember what I was thinking here -- maybe it snuck in while I was testing something. Without re-familiarizing myself with the code, I'd say that conditional should be removed so that the add link always shows when the show op links preference is set.

BBC’s picture

Thanks. The operation links are there. Tried with both default and custom link titles.

I've also tried removing the conditional per your suggestion and am getting the same results. I have yet to see an edit or delete link appear since applying the patch and am only able to get an add link to appear in situations where there's no data in the field collection.

Would really like to see this feature get implemented, but am going to have to roll back and try a different

RobW’s picture

I haven't had time to look at this patch since the re-roll, but I'm going to have to next week for a client project. I'll report back then, but it could be a while. In the meantime the code is pretty well documented if someone wants to jump in and debug themselves.

RobW’s picture

OK, I re-rolled against the latest dev and tested. On a fresh Drupal install this patch works, displays the operation links correctly, etc. After reading through the code again, I'm sure the if (!$items) check is correct and should stay in -- it does what it says in the comment, attaches the add operation link if there are no items. If there are items, the call to the operation link helper function inside the items loop attaches the add link.

This patch also fixes a minor bug in the formatter_settings_summary, where some isset() checks were missing.

dkre’s picture

With patch #74 the template seems a little borked.

I don't get any operator links at all.

The containers are all collapsed (field-item) and the

.. in field-collection-item.tpl.php needs a clearfix div inserted after the print render($content) so the container wraps all of the fields within it.

Previous styling also seems to be gone - dotted border below etc.

Should note this isn't with a clean install.

kscheirer’s picture

Category: bug » task
Status: Needs review » Needs work

Moving this to a task, not a bug report. I'm trying to clear out bugs to get to a 1.0 release, and this will go into 2.x. Also needs work, based on #74.

RobW’s picture

To #75: There are no floats in the revised field collection css, so I'm betting your issues are site specific. Let me know if you can reproduce any problems on a clean install.

Also, haven't been here in a few months so this patch is probably due for a code review and re-roll, and revision if #1281272: Make inline admin links on field_collection_view into contextual links has buy in form the current maintainers (any comment?).

rooby’s picture

Re #76:

Is this definitely not getting into 1.0?

Since the current implementation of field_collection_item.tpl.php doesn't work does that mean that we have to wait for who knows how long until 2.0 comes out until we can theme field collection items?

I would say this should be a 1.0 blocker (and a bug), unless it's just me who can't override the themeing of a field_collection_item.

rooby’s picture

Here is a re-roll of #74 that applies cleanly to latest dev.

rooby’s picture

Sorry, that one was missing the newly added files.
This one has it all:

rooby’s picture

Here is a quick readthrough review.
I have not tested functionality wise yet.

I will re-roll with these minor fixes.

Also, seeing as a formatter (field_collection_fields) is being removed this will need a large notice to tell people about the change.
Also, maybe an upgrade path for people using that one could be investigated?

There needs to also be a notice about theme_field_collection_view going away.

+++ b/field_collection.module
@@ -753,7 +753,8 @@ function field_collection_permission() {
- *   The operation being performed. One of 'view', 'update', 'create', 'delete'.
+ *   The operation being performed. One of 'view', 'edit',
+ *   'add', or 'delete'.

That comment can fit on a single line.

+++ b/field_collection.module
@@ -775,25 +776,45 @@ function field_collection_item_access($op, FieldCollectionItemEntity $item = NUL
+  // Entity access takes 'view', 'update', 'create', or 'delete' operations.

This comment does not match the change to the $op param in the function doc block.

+++ b/field_collection.module
@@ -775,25 +776,45 @@ function field_collection_item_access($op, FieldCollectionItemEntity $item = NUL
+//RW still need both of these?
   return array(
     'field_collection_item' => array(
       'render element' => 'elements',
+      'path' => $path . '/theme',
       'template' => 'field-collection-item',
     ),
-    'field_collection_view' => array(
-      'render element' => 'element',
-    ),
+//    'field_collection_view' => array(
+//      'render element' => 'element',
+//    ),

I would think we would not need both, as per your commenting.

+++ b/field_collection.module
@@ -1099,51 +1116,73 @@ function field_collection_field_formatter_settings_form($field, $instance, $view
+  // @TODO: Apply view modes of field collection parent entity directly, with no formatter.

Line too long.

+++ b/field_collection.module
@@ -1155,24 +1194,24 @@ function field_collection_field_formatter_settings_summary($field, $instance, $v
+      isset($links['add']) && isset($settings['description']) ? $help_text = ' with field help text' : $help_text = '';

@@ -1182,117 +1221,196 @@ function field_collection_field_formatter_view($entity_type, $entity, $field, $i
+  if ($display['type'] == 'field_collection_items' || 'field_collection_links') {
+
+    // Get view mode for template suggestions.

I feel this would be more readable as:

$help_text = (isset($links['add']) && isset($settings['description'])) ? ' with field help text' : '';
+++ b/field_collection.module
@@ -1182,117 +1221,196 @@ function field_collection_field_formatter_view($entity_type, $entity, $field, $i
+  if ($display['type'] == 'field_collection_items' || 'field_collection_links') {
+

My preference would be to keep the switch statement instead of using if (and possibly more ifs in future), although it doesn't matter too much.

+++ b/field_collection.module
@@ -1182,117 +1221,196 @@ function field_collection_field_formatter_view($entity_type, $entity, $field, $i
+    $view_mode = !empty($display['settings']['view_mode']) ? $display['settings']['view_mode'] : 'full';
+
+    foreach ($items as $delta => $item) {
+
+      // Check field collection items exist and load entity info.
+      if ($field_collection_item = field_collection_field_get_entity($item)) {
+
+        $item_id = implode($item);
+
+        // Load render arrays of the collected fields.
+        $field_collection_item_render = $field_collection_item->view($view_mode);
+
+        if ($display['type'] == 'field_collection_links') {
+
+          // Remove fields and add links to the field collection item entities.
+          foreach ($field_collection_item_render['field_collection_item'] as $eid => $render_array) {
+

Lots of blank lines, are they really all necessary?

I think it actually hinders readability to have too many blank lines.

This happens elsewhere in the file too.

+++ b/field_collection.module
@@ -1182,117 +1221,196 @@ function field_collection_field_formatter_view($entity_type, $entity, $field, $i
+ * Impliments hook_preprocess_HOOK().

Typo in implements.

+++ b/field_collection.module
@@ -1182,117 +1221,196 @@ function field_collection_field_formatter_view($entity_type, $entity, $field, $i
+    // Add op link properties as template variables

No full stop at the end of the sentance.

+++ b/theme/field-collection-item.tpl.php
@@ -0,0 +1,83 @@
+ ¶

Trailing space.

+++ b/theme/field-collection-item.tpl.php
@@ -0,0 +1,83 @@
+      <?php
+        print render($content);
+      ?>

This could be on a single line.

+++ b/theme/field_collection.theme.css
@@ -0,0 +1,48 @@
+  text-align:right;

No space after the colon.
This happens a number of times through this file.

+++ b/theme/field_collection.theme.css
@@ -0,0 +1,48 @@
+.field-collection-op-link:last-child  {

Extra space before opening brace.

+++ b/theme/field_collection.theme.css
@@ -0,0 +1,48 @@
+}
\ No newline at end of file

No newline at end of file.

RobW’s picture

Thanks for the code review, rooby. Not sure when I'll be able to get back to this patch, so if anyone else wants to make the code changes please feel free.

rooby’s picture

I have already made the changes in my review. Just doing some testing and hopefully post the patch today.

rooby’s picture

Status: Needs work » Needs review
FileSize
33.14 KB

Here is the patch with the fixes for #81 plus a couple of other super minor coding standards things I missed in #81.

I can confirm it is working.

As a side note, to override the field-collection-item.tpl.php you need to use the development version of the entity module - See #1462772: template suggestions are not working if no custom template is defined

That also likely explains why the template file wasn't working for me before, so I retract my statement from #78 regarding the current themeing not working.

Either way I still vote for this getting into 1.x as opposed for waiting for 2.x.

Status: Needs review » Needs work

The last submitted patch, field_collection-add_tpls_all_in_one-1157794-84.patch, failed testing.

rooby’s picture

It was initially ok for my testing as I wasn't using the add/edit/delete links.

When I went to go fix the test that failed I realise the links are not working properly.

I will investigate a little later. Probably tonight.

RobW’s picture

I agree that this is an essential Field Collection feature, but it completely breaks current theming on 1.x. I would love to see Field Collection have a Photoshop 5.5 style half release with this in it while 2.x is still in early development.

rooby’s picture

Yeah, I understand where you're coming from.

In my experience it didn't break things badly but it did a little, and with 30000+ users it's probably not a good idea to push into the 1.x branch.

dkre’s picture

#77 RobW - Too true,

I did post this a little too quickly at the time, I'm much better at qualifying my issue posts.

This was with an Omega XHTML site, I will definitely look into this further on my next FC build.

ianthomas_uk’s picture

This looks to me like two separate, if very closely related, tasks:
* Move markup to template files; and
* improve theming

Am I correct in thinking that the reason the patch isn't suitable for 7.x-1.x is because it changes the output HTML to improve the theming? If so, is there any reason we can't move the markup to template files while maintaining the current theming? This could even allow an upgrade path to improved markup, but shipping with new and old template files, so people could easily switch between the two behaviours. I'm still getting my head around the theming layer, so apologies if I've missed something obvious that would prevent that.

RE having a "point 5" release - a 2.0 release doesn't need to have huge changes, it's just a warning to developers that it's not backwards compatible. The disadvantage is you either need to deprecate the 1.0 branch or spend extra time maintaining two separate branches.

wizonesolutions’s picture

So can this be part of a 7.x-2.x-dev branch then? Sorely needed.

Is field-collection-item.tpl.php currently picked up? I see that it's defined, but I don't see any calls to theme('field_collection_item') in Field Collection. As RobW says, everything looks to be wired up in the formatter.

rflnogueira’s picture

Any update to this issue? Theming Field Collections is a real PITA...

wizonesolutions’s picture

Nope, still a PITA. I wrote a set of preprocessing functions with one client that is still working pretty well. Start from hook_preprocess_field() and hook_preprocess_entity() and go from there.

fago’s picture

Title: Move markup to template files and improve theming » Move markup to template files and improve theming in new 2.x branch

Yeah, I think the approach makes a lot of sense and is better.

This means we can't put this into 7.x-1.x. It'd have to be 7.x-2.x. Assigning to fago for his opinion.

After discussing the issue with some folks I agree that we cannot do this in 1.x. Still I'd like to move on with this if possible. Any suggestions on this would work out best?

E.g., roll another beta release in 1.x asap, then start with 2.x from there. Then, in future we'd only continue maintaining 2.x and provide security fixes for 1.x only. Or should we backport fixes to 1.x if they apply or someone steps up for them? Howsoever, from that point 2.x should be the mainly moving branch and every patch would need to go in 2.x first - at least. Thoughts?

wizonesolutions’s picture

That's how I handle it in FillPDF. 7.x-2.x is where everything has to go first (unless someone is paying me for the work, of course). And then people can backport stuff if they want, or I might in some cases, but generally 7.x-1.x is not where new things happen first.

wizonesolutions’s picture

Issue summary: View changes

Grammar

jmuzz’s picture

See next comment.

jmuzz’s picture