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.
Comments
Comment #1
fagoMakes sense, patch welcome!
Comment #2
tim.plunkettComment #3
dman CreditAttribution: dman commentedYeah. Good request.
Comment #4
RobW CreditAttribution: RobW commentedOK, 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?
Comment #5
RobW CreditAttribution: RobW commentedOK, 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.
Comment #6
dman CreditAttribution: dman commentedAs 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.
Comment #7
RobW CreditAttribution: RobW commentedI 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.
Comment #8
RobW CreditAttribution: RobW commentedMy 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.
Comment #9
RobW CreditAttribution: RobW commentedRunning 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.)
Comment #10
RobW CreditAttribution: RobW commentedDecided 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.
Comment #11
RobW CreditAttribution: RobW commentedOK, 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
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
If anyone has opinions, let me know.
Comment #12
RobW CreditAttribution: RobW commentedHere's an incomplete patch for testing and suggestions.
Done right now:
field_collection_view
theme wrapper. Added its classes and attributes to field-collection-item.tpl.php, along with some other revisions.Left to do:
Comment #13
RobW CreditAttribution: RobW commented[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:
Using a foreach would solve some problems and simplify a lot of things:
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.#suffix
hack for add links I mentioned in #9.Downsides:
Am I missing an obvious downside? If anyone has thoughts let me know; I'm going to start refactoring today.
Comment #14
RobW CreditAttribution: RobW commentedThat 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.
Comment #15
nairb CreditAttribution: nairb commentedThe 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
Comment #16
RobW CreditAttribution: RobW commentedSome more notes:
The add link presents a bunch of problems. There are three ways we can attach it to the field collection:
#suffix
. This is the pre-patch placement.Result:
Dealbreaker: the add link is inserted after and outside of all field markup, breaking document structure and css theming.
#suffix
.Result:
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.
field__field_collection
theme function or template, and insert the add link in a custom variable.Result:
Perfect placement! But, Dealbreaker: modules and themes that provide their own
theme_field
implementation (e.g. base themes) will have to override boththeme_field
andtheme_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.
Comment #17
RobW CreditAttribution: RobW commentedHere's an updated patch for anyone who's interested in progress on this issue. The new stuff:
field-collection-op-links.tpl
. The op links are now provided as variables infield-collection-item.tpl
. It adds more stuff to the field collection item template, but overall I think it makes theming and code maintenance easier.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?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!
Comment #18
RobW CreditAttribution: RobW commentedHere 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:
field-collection-item.tpl.php
.classes_array
now.)Comment #19
RobW CreditAttribution: RobW commentedScreenshots of the new formatter options, and the css with the items and list formatters:
Comment #20
RobW CreditAttribution: RobW commentedAnd here's what the template file looks like:
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.
Comment #21
RobW CreditAttribution: RobW commentedComment #23
RobW CreditAttribution: RobW commentedAh, 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.
Comment #25
dman CreditAttribution: dman commentedbravo!
I'll have to try it out..
Comment #26
RobW CreditAttribution: RobW commentedI 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.
Comment #27
dman CreditAttribution: dman commentedPatch 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...
Comment #28
paulpaulti2 CreditAttribution: paulpaulti2 commentedJust 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 :)
Comment #29
paulpaulti2 CreditAttribution: paulpaulti2 commentedHi,
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
Regards,
paul
Comment #30
RobW CreditAttribution: RobW commentedThanks 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.
Comment #31
paulpaulti2 CreditAttribution: paulpaulti2 commentedHey 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.
Comment #32
RobW CreditAttribution: RobW commentedRevised tpl documentation to include zebra and id.
Comment #34
RobW CreditAttribution: RobW commented#32: field-collection-add-tpls-all-in-one-1157794-32.patch queued for re-testing.
Comment #36
RobW CreditAttribution: RobW commentedEh, well it passes simpletest on my local machine. I think the testbot is just having some issues.
Comment #38
dman CreditAttribution: dman commentedIt's complaining about the patch format itself, not the tests.
Though I can't visually see what it's actually complaining about. Maybe it needs a re-roll against -dev?
Comment #39
RobW CreditAttribution: RobW commentedAh, that's a new message. Before it was giving me "could not initialize mysql environment" or something similar.
Comment #40
RobW CreditAttribution: RobW commentedRebased. Let's try this one.
Sorry for all of the junk comments.
Comment #41
RobW CreditAttribution: RobW commentedComment #41.0
RobW CreditAttribution: RobW commentedUpdated the issue summary with all changes up to comment #40
Comment #42
RobW CreditAttribution: RobW commentedMarked #1187990: Is there a node--field-collection-fieldcollectionname.tpl.php template? and #1461734: field_collection.theme.css not needed? as a duplicates.
Comment #43
jisuo CreditAttribution: jisuo commentedHey 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.
Comment #44
RobW CreditAttribution: RobW commentedThe 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.
Comment #45
tim.plunkettPutting back on my radar.
RobW, thank you for all of your work here!
Comment #46
dman CreditAttribution: dman commentedI'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.
Comment #47
tim.plunkettSo, thi
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.
Comment #48
RobW CreditAttribution: RobW commentedI was thinking that would be the case. Is there a Drupal way to write a script to update a user's settings?
Comment #49
tim.plunketthttp://api.drupal.org/api/search/7/hook_update_N
Comment #50
RobW CreditAttribution: RobW commentedThanks, Tim. So it looks like you can't do schema updates inside of a major module version, which I guess makes sense.
Comment #51
tim.plunkettYou 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.
Comment #52
RobW CreditAttribution: RobW commentedI am. Send me a PM -- I could schedule some time this Fri, Sat, or late Sunday.
Comment #53
jisuo CreditAttribution: jisuo commentedRobW: I tried it out. Created several field collections added different tpl-files and everything seemed to work as intended. Good work!
Comment #54
gillarf CreditAttribution: gillarf commentedGreat 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
Comment #55
RobW CreditAttribution: RobW commentedYeah, 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.
Comment #56
fagoWell, 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.
We should keep sanity in the possible operations and either go with 'edit' or 'update', 'add' or 'create' but not both.
Comment #57
RobW CreditAttribution: RobW commentedThanks 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.Comment #58
gillarf CreditAttribution: gillarf commentedThanks 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!)
Comment #59
RobW CreditAttribution: RobW commentedHere's a comparison of output before and after the patch, with notes.
Current Markup:
New Markup:
Comment #60
jnettikI 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.
Comment #61
fagoI 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.
Comment #62
junedkazi CreditAttribution: junedkazi commentedIf 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.
Comment #63
junedkazi CreditAttribution: junedkazi commentedThe 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 ??
Comment #64
marcusx CreditAttribution: marcusx commentedTrying to retest as #1782664: Call to undefined function entity_revision_is_default() is fixed.
Comment #65
marcusx CreditAttribution: marcusx commentedEDIT:
Mmmm - how can I reactivate a "Postponed" test?
Comment #66
junedkazi CreditAttribution: junedkazi commentedComment #67
junedkazi CreditAttribution: junedkazi commentedTest back in queue.
Comment #68
marcusx CreditAttribution: marcusx commentedMmm not working. Still postponed. Uploading patch again with new name.
Comment #69
marcusx CreditAttribution: marcusx commentedComment #70
BBCMany 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.
Comment #71
RobW CreditAttribution: RobW commentedDid 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:
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.
Comment #72
BBCThanks. 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
Comment #73
RobW CreditAttribution: RobW commentedI 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.
Comment #74
RobW CreditAttribution: RobW commentedOK, 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 someisset()
checks were missing.Comment #75
dkre CreditAttribution: dkre commentedWith 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.
Comment #76
kscheirerMoving 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.
Comment #77
RobW CreditAttribution: RobW commentedTo #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?).
Comment #78
rooby CreditAttribution: rooby commentedRe #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.
Comment #79
rooby CreditAttribution: rooby commentedHere is a re-roll of #74 that applies cleanly to latest dev.
Comment #80
rooby CreditAttribution: rooby commentedSorry, that one was missing the newly added files.
This one has it all:
Comment #81
rooby CreditAttribution: rooby commentedHere 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.
That comment can fit on a single line.
This comment does not match the change to the $op param in the function doc block.
I would think we would not need both, as per your commenting.
Line too long.
I feel this would be more readable as:
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.
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.
Typo in implements.
No full stop at the end of the sentance.
Trailing space.
This could be on a single line.
No space after the colon.
This happens a number of times through this file.
Extra space before opening brace.
No newline at end of file.
Comment #82
RobW CreditAttribution: RobW commentedThanks 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.
Comment #83
rooby CreditAttribution: rooby commentedI have already made the changes in my review. Just doing some testing and hopefully post the patch today.
Comment #84
rooby CreditAttribution: rooby commentedHere 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.
Comment #86
rooby CreditAttribution: rooby commentedIt 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.
Comment #87
RobW CreditAttribution: RobW commentedI 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.
Comment #88
rooby CreditAttribution: rooby commentedYeah, 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.
Comment #89
dkre CreditAttribution: dkre commented#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.
Comment #90
ianthomas_ukThis 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.
Comment #91
wizonesolutionsSo 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.Comment #92
rflnogueira CreditAttribution: rflnogueira commentedAny update to this issue? Theming Field Collections is a real PITA...
Comment #93
wizonesolutionsNope, still a PITA. I wrote a set of preprocessing functions with one client that is still working pretty well. Start from
hook_preprocess_field()
andhook_preprocess_entity()
and go from there.Comment #94
fagoYeah, I think the approach makes a lot of sense and is better.
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?
Comment #95
wizonesolutionsThat'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.
Comment #95.0
wizonesolutionsGrammar
Comment #96
jmuzz CreditAttribution: jmuzz commentedSee next comment.
Comment #97
jmuzz CreditAttribution: jmuzz commentedAdding to #2444471: [Meta] Field collection 7.x-2.x branch.