In the Field module in field.module for the method field_bundle_settings the comment states an example for the array structure. For view_modes the example is view_modes->full->custom_display, but it should be custom_settings instead, since that is being used as you can see for example in the method field_view_mode_settings (line 653ff.).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklas Fiekas’s picture

Version: 7.12 » 8.x-dev
Status: Active » Needs work
Issue tags: +Novice, +Coding standards

Thanks, Volx!

Moving to 8.x. Patches need to get into D8 first and are then backported. Needs work and +Novice for the straight forward reroll.

jhodgdon’s picture

Component: field system » documentation
Issue tags: -Coding standards

fixing component and tags

Chaulky’s picture

Rerolled the patch against current 8.x (commit: 9e8d1e85) and 7.x (commit: f10acf95).

Status: Needs review » Needs work

The last submitted patch, field_doc.patch, failed testing.

Chaulky’s picture

Oh yea... shouldn't ignore just because it's documentation. Still needs to actually apply correctly. Reattached patches with proper names

Chaulky’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

You know, this whole documentation block is written in a non-standard way.

First off, it is describing the $settings parameter, so it should be part of the @param $settings section, not its own paragraph.

Second, normally we document structures like this using the documentation list format (see http://drupal.org/node/1354#lists), something like this (will need to be wrapped appropriately):

* @param $settings
*  (optional) The settings to store, an associative array with the following elements:
*  - view_modes: An associative array whose keys are names of view modes, and whose values are associative arrays with the following elements:
*     - custom_settings: TRUE if the view mode uses custom settings.
*  - extra_fields: ...

Could someone please make a patch with this rewrite in it? Thanks! And as usual, I would suggest just doing 8.x to start, and waiting until we have an accepted patch before embarking on 7.x (saves time and effort).

Chaulky’s picture

Status: Needs work » Needs review
FileSize
3.21 KB

Removed the code example from the description section of the function documentation. Greatly enhanced the @param entry for $settings and added types to all @params and @return according to new D8 standards (http://drupal.org/node/1354#functions). Patched against latest 8.x (commit: faa63d22).

The language for $settings feels a little awkward to me, mostly because there are just so many arrays at work here. I think this patch gets the point across, but I'm open to suggestions for improvement.

Niklas Fiekas’s picture

Thank you, Chaulky. That looks awesome.

+++ b/core/modules/field/field.moduleundefined
@@ -536,51 +536,33 @@ function _field_sort_items_value_helper($a, $b) {
+ *       - visible: TRUE if the extra field is visible, FALSE otherwise

The missing . is the only flaw I could find. @jhodgdon: Can you find anything else? Otherwise I'd say RTBC, after that is fixed.

jhodgdon’s picture

Status: Needs review » Needs work

I noticed this:

+ *       - weight: The weight of the extra field, determining it's position on
+ *         on an entity form.

it's -> its

and this:

+ *       - weight: The weight of the extra field, determining it's position when
+ *         an entity if viewed.

it's -> its; if -> is

Otherwise (and the missing period noted above) -- looks great, thanks!

Chaulky’s picture

I've made the corrections from #9 and #10. I also attached a patch for D7 that makes the exact same changes. I left in the types on @params and @return even though technically that wasn't the standard recommendation in D7. I can remove those for D7 if necessary, but thought it couldn't hurt to leave it in.

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7
FileSize
1.44 KB

RTBC, because the interdiff looks just as good as the patch.

Chaulky’s picture

Now that it's marked for backport to D7, should I attach the D7 patch with the correct name for testing and switch the version over to 7.x? Technically, the changes are exactly the same unless we want to remove the types from @params and @return.

Niklas Fiekas’s picture

Let's first wait until it's committed. That's better than switching back and forth between 7.x and 8.x.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Right, it's best to wait until it's committed. Much of the time, the 8.x patch can be directly applied to 7.x, and you don't even have to create a 7.x patch. In other cases, the committer can find the 7.x patch with the odd name, and go ahead and commit it (especially if it's just documentation, not much worry about breaking core etc.).

Speaking of which... I just committed this to both 8.x and 7.x. Thanks!

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