Merge Field Formatter Order sandbox with Field Multiple Limit

+1
Actually I came here looking for a 'reverse order by delta' option - which combined with limit=1 would produce this.

I'm seeing if my sandbox
Field Formatter Order which allows you to reverse the delta order will be practical.
Right now, yes, if you reverse and limit the display to 1, you will get the last item!

Reverse order is a really cool idea. I wasn't very warm to the idea of 'last item' as an option, but 'reverse order' offers a lot more flexibility. Effectively, it lets you choose between FIFO and LIFO stacks. Sweet!

I look forward to seeing the sandbox code. The #weight issue sounds a bit awkward.

As for this feature request, the new module effectively means it's fixed. Going forward, would you prefer to merge this functionality with Field Multiple Limit, or promote Field Formatter Order to a full project?

Some points to consider:
1. Field Formatter Order is a really good module name - 'multiple limit' doesn't really hint at order-changing functionality, and 'formatter order' could accommodate other options, like random-order.
2. Field Formatter Order may well turn out to useful on it's own, without multiple limit.
3. Field formatter settings modules do tend to clutter up the manage-fields form once there's a bunch of them installed, and keeping the modules separate might help reduce that.
3. Field Multiple Limit already has a D8 version, so less work remaining on that front.
4. Field Multiple Limit already has a fairly decent-sized user base, so merging could help get the reverse-order feature out to more users quicker.

Hope you don't mind me changing the issue title and moving this to your sandbox. If you want to join forces, can accept a patch and/or let you have commit access. If you want to make Field formatter order a full project, you can just close this (works as designed).

I would be keen for this to merge.
The 'meat' of the code is only a dozen lines, including the form, the rest is just the structure around the API, so it would be much cheaper to merge.
I see these two functions as being very closely related conceptually, and it feels like overkill to have them in different spaces - especially with the formatter settings API already an additional dependency.

I did consider 'random' as a later option when naming. I knew 'reverse' would be a *little* too limiting.

I got distracted when tidying up the description so forgot to push the code - I'll get that up when I'm back at my machine.

I'll put up the sandbox code still, but also provide a patch (which will be 1/3 of the size of the full module) that could be added to field_multiple_limit.
Not sure how much to worry about 'clutter' in the UI. I like using 'fieldgroup' and that really piles in the options, so I think it can be done well.

Sandboxy code pulled out of my dev box and thrown up to https://drupal.org/sandbox/dman/2073991 as a start.

@rooby, see my screenshot, and yes, we can see this minor feature coming in to be just the one module.
Sandbox is just that ...

@rooby

Experienced developers try to look at the big picture and solve the general problems, even if todays problem is a special case.
The Unix way is to make small tools that do one thing well with appropriate actions, and then chain them.

This is sorta similar. A 'limit number' tool and a 'reverse order' tool are both useful in themselves, and if you want 'just the first two' or 'just the last two' then these tools can be combined and become greater. Today, I want 'just the last 5', and you want 'just the last one'. These are all possible now.

I have built a tool that serves my purpose, and its utility multiplied when combined with field_multiple_limit, and you win from it, even though that was not our (primary) goal.
Your 'last only' requirement is now a subset of what can be done.

The sum is greater than the parts alone! Synergy!

@rooby - dman's sandbox code does indeed just add an extra checkbox to reverse item order.

@dman - code looks reasonable at first glance, lets's use the remainder of this issue to work on a patch. Updated issue title slightly :-)

hook_field_attach_view_alter() is the meaty part.
The other hooks are just a case of rolling in the extra settings.

My sandbox code is not tuned. I tried 6 different methods to do sensible element_children re-ordering before I fell back to this least-worst version.
I'd appreciate a sanity-check, as my willpower was worn out from two hours tracing why theme_field and the dozen funcs were not using drupal_render conventions...

Okay, but probably needs to wait until the weekend before I can look at it ;-)

Just been reviewing the other outstanding issues with this module, and noticed that there is already a feature request for random order!

#1866410: Allow for randomized items

Postponing that issue, until after this one.

Yay. I've not re-rolled yet - got a lot on at the moment, but it's on my list..

I have a patch. Switching back to the main queue so I can submit it right.

Only a few lines here and there, this is much more economical than a stand-alone module

I couldn't sleep tonight, so I added some unit tests too.

bump?

This looks pretty good to me. Tested it out and everything seems to be in order. I made so notes below, but they are all pretty minor points.

1. +++ b/field_multiple_limit.module
   @@ -18,6 +18,7 @@ function field_multiple_limit_field_formatter_info_alter(&$info) {
   +    $formatter['settings']['field_formatter_order'] = FALSE;
   

   field_formatter_order doesn't follow the naming conventions for the rest of the formatter settings. Should be field_multiple_limit_order instead

2. +++ b/field_multiple_limit.module
   @@ -42,6 +43,12 @@ function field_multiple_limit_field_formatter_settings_summary_alter(&$summary,
   +
   +
   

   remote extra blank lines

3. +++ b/field_multiple_limit.module
   @@ -82,9 +96,21 @@ function field_multiple_limit_field_attach_view_alter(&$output, $context) {
   +
   

   remove extra blank line

4. +++ b/field_multiple_limit.test
   @@ -0,0 +1,120 @@
   +    // Check that the fields are displayed in the normal order
   +    $this->assertPattern('|Field data #0[\s\S]*Field data #1[\s\S]*Field data #2[\s\S]*Field data #3|', 'All fields are displayed in delta order on the page.');
   

   This doesn't actually test that all the fields are present, you'd need to look for #4 as well. It could lead to a false positive if this didn't display #4 for some reason, then you set the limit to 3 and still don't get #4.

Yep, all good.
The naming thing makes sense if it's up for a merge. I can adapt that.
Random whitespace is obviously inadvertent due to the copy&paste merging.
It's sorta-by-design and/or lazy that I didn't check more than three out of four possible - the failure would be a bizarre edge case, 3 items in correct order should have proved the point that the ordering was working as expected... but I can fill that in anyway for consistency, sure.

I was away (Drupalcon etc) for the last month, and just revisited our local ticket system where this little feature was waiting for my follow-up. I can re-roll now...

variable renamed, some whitespace removed, test updated as suggested

@dman Yea i was definitely being quite picky about the tests. I've been fixing and writing missing tests at work lately so I got into a mood about it. Thanks for bearing with me on that.

This looks good to me. I didn't do much to research a different way to do the element reversal, but your method seems reasonable to me. Since you mentioned trying for a while to come up with a different way I'd say what you have now is a good way to go about it. I'd called this RTBC.

While I have commit access, I've been away from the project for quite a while so I don't want to just jump in and start committing stuff. I'll wait for @andrewmacpherson to see this too.

Patch still applies clean today.
Bump?

I was releasing yet another field formatter setting https://drupal.org/project/field_formatter_filter this week and just came back to find this wasn't in yet. :-(

Actually, there was stray whitespace in that patch. re-roll

aand - my re-roll omitted the (new) test file. re-re-roll.

Hi, I can confirm that #23 works. I needed the random order option, so I added that and updated the test. Sorry if this throws off the workflow.

aaand again with the new test file.

Corrects the default setting for field_multiple_limit_order to 0 instead of FALSE.

O_v on #25.

Visually looks good. Random order may as well go in now, it looks tight and consistent enough.
Did not know that there is actually a *test* for 'random order' (how is assertNoPattern even possible?)
But thanks. Still looking good to me. (visual review only just now)

Thanks for keeping this going @dman and @awolfey. I'll give this a review and some testing tomorrow and we'll make this happen!

For the random order test, I'm only asserting that it's not in the original order. There will no doubt be false positives, especially with the low number of items in the list. It would probably be better to run that part of the test multiple times to make sure.

@awolfey - my spidey-senses tell me that's asking for trouble. A test that *randomly* fails one time in 24?

Here's a better test. The same random pattern 10 times in a row will be a false positive. What are the odds?

OTTOMH,
4! ^ 10 = 63,403,380,965,376
... I guess? Though maybe it's just ^9 as the seed order is irrelevant.
So OK, unlikely.

We're pretty close here. Just a few minor changes and I think we can put it in. One thing I'm still a little confused about is why the random order works without specifying weights. The keys end up getting sorted somewhere, which puts the values in the intended random order even without the weights, but I haven't tracked that down yet. In fact, I tried adding incorrect weights and still got the intended random order. I wouldn't necessarily hold this because of that, but I'd like to wrap my head around it.

1. +++ b/field_multiple_limit.module
   @@ -9,6 +9,9 @@
   +define('FIELD_MULTIPLE_LIMIT_ORIGINAL', 0);
   +define('FIELD_MULTIPLE_LIMIT_REVERSE', 1);
   +define('FIELD_MULTIPLE_LIMIT_RANDOM', 2);
   

   Let's make these FIELD_MULTIPLE_LIMIT_ORDER_XXX to make it more clear that they're not actually about limits, but order.

2. +++ b/field_multiple_limit.module
   @@ -18,6 +21,7 @@ function field_multiple_limit_field_formatter_info_alter(&$info) {
   +    $formatter['settings']['field_multiple_limit_order'] = 0;
   

   Use the constant instead of '0'

3. +++ b/field_multiple_limit.module
   @@ -42,6 +46,22 @@ function field_multiple_limit_field_formatter_settings_summary_alter(&$summary,
   +        case FIELD_MULTIPLE_LIMIT_ORIGINAL:
   +          $summary .= '<br/>' . t('Original Order');
   +          break;
   

   This case will never be used. If it's sets to use the original order, the empty() call with return true, skipping the whole switch statement. Either remove the if statement or make this the default case in the switch. Or, remove the case and don't display anything in the summary (like we do for offset). What do you think is cleanest? I'd vote to just remove the case and display nothing for original order, like we do for offset.

4. +++ b/field_multiple_limit.module
   @@ -65,6 +85,16 @@ function field_multiple_limit_field_formatter_settings_form_alter(&$settings_for
   +      '#default_value' => $settings['field_multiple_limit_order'] ? $settings['field_multiple_limit_order'] : 0,
   

   I don't think we need the check here. We set the default to 0 (using the constant) which would be false and set #default_value to 0. Without the check, it would still be set to 0.

5. +++ b/field_multiple_limit.module
   @@ -82,9 +112,38 @@ function field_multiple_limit_field_attach_view_alter(&$output, $context) {
   +      case FIELD_MULTIPLE_LIMIT_RANDOM:
   +        $shuffled = element_children($element);
   +        shuffle($shuffled);
   +        $resorted_items = array();
   +
   +        foreach (element_children($element) as $element_child_id) {
   +          $resorted_items[$shuffled[$element_child_id]] = $element[$element_child_id];
   +          unset($element[$element_child_id]);
   +        }
   +        $element += $resorted_items;
   +        break;
   

   Why don't we need to set weights here? In fact, why does setting weights not seem to affect it?

Short answer - #weights don't work at all.
That's why I lost a few hours on it myself in the beginning. It just don't.

As I described, on https://drupal.org/sandbox/dman/2073991, and signposted with a comment in the code - the only way was to re-order the keys and walk away.
For consistency, I'd be happy with setting a (useless) weight, but field renderer does not sort on it. I consider that an oversight in core.

#3 I'd like to see it as just the switch:default.

Let's be sure to add in the weights. I experimented a bit this weekend and we should be able to do the sort ourselves in a preprocess hook, which will run after template_preprocess_field() and let us sort the items in the $variables for theming.

Found a PHP notice issue when using a field in a view that was hidden from the default display mode. Attached is a version of the patch in #31 that fixes that, and below is a diff between the two patches:

diff --git a/field_multiple_limit.module b/field_multiple_limit.module
index 921a966..abffa3b 100644
--- a/field_multiple_limit.module
+++ b/field_multiple_limit.module
@@ -112,10 +112,11 @@ function field_multiple_limit_field_attach_view_alter(&$output, $context) {
 
     $limit = empty($info['field_multiple_limit']) ? FIELD_MULTIPLE_LIMIT_ALL : $info['field_multiple_limit'];
     $offset = empty($info['field_multiple_limit_offset']) ? 0 : $info['field_multiple_limit_offset'];
+    $order = empty($info['field_multiple_limit_order']) ? FIELD_MULTIPLE_LIMIT_ORIGINAL : $info['field_multiple_limit_order'];
     $element = &$output[$field_name];
 
     // Re-ordering comes before limiting.
-    switch ($info['field_multiple_limit_order']) {
+    switch ($order) {
       case FIELD_MULTIPLE_LIMIT_ORIGINAL:
         break;

Hiding some older patches.

This addresses the review from #33 and adds fix from #36.

Bump?
anything holding this up still?

This looks good to me. Tests all pass, code looks clean, manual testing checks out. Will work on getting this committed shortly.

@chaulky, anything I can do with this to help?

I'm now helping out maintaining this module, and I've published this patch to the dev version. A new alpha release with the patch is also forthcoming.