Use case: image field set to unlimited, but you only want to show the first on a teaser view. Inspired by http://realize.be/limit-number-fields-display-field-ui-display-suite and #1234618: Limit the number of fields to display. Patch forthcoming for Drupal core.

Comments

yched’s picture

subs

swentel’s picture

FileSize
4.15 KB
FAILED: [[SimpleTest]]: [MySQL] 33,601 pass(es), 0 fail(s), and 15 exception(es). View
21.6 KB
27.22 KB

Ok, first version of a patch, still needs work, but setting to needs review for feedback. I've attached some screenshots as well so people can have a first impression.

Things that need work:
- upgrade path: save all instance settings with limit ?
- tests: upcoming

Feedback:
- is the preprocess_field the right place or should we cut of items earlier ?

(second try - stupid double click)

pcambra’s picture

Great idea indeed, just a random thought, maybe this should belong to the formatting options

droplet’s picture

missing a way to view all images ?

swentel’s picture

@droplet: if you leave that field empty, it shows nothing.
@pcambra: possibly. Although at this point we can only form alter into that form because it's a module specific thing (unless I've missed a hook). We could introduce a new hook or another technique of course.

Also, I've been thinking about the placement of this. Maybe we could even introduce '#display-limit' as a property for any sort of element and let drupal_render() handle this in the end. Thoughts about that ?

pcambra’s picture

yched’s picture

Status: Active » Needs review
Issue tags: +Needs usability review

Patch is fine code-wise.
Obviously the main challenge here will be to find the right UI shape. Thus, added the 'UX review tag'. I know UX folks *love* a good brainstorm on the Field UI :-D.

We're talking about generic settings that are relevant independently of the field type and formatter, as opposed to the existing formatter settings.
For the record, and if that brings food for the discussion : other much awaited features, in addition to this "# of values to display" setting, include :
- custom markup aka "semantic fields", currently also provided by Display Suite D7 (@swentel, maybe you have a screenshot of the current UI somewhere ?)
- separator for multiple values if the markup for multiple values is inlined (comma, dash, ...)
Of interest (or not ?) for the UI considerations, all those features pertain to multi-values fields, much less to single-value fields (or non-fields)

Contrib D7 currently already clings those features (exceot maybe the 'separator' one ?) as it can on the core Field UI screen - so this is not a hypothetical case. As the above comments show, *2* different contrib modules currently provide the '# of values to display' feature.

swentel’s picture

re: that markup, here's the screenshot http://realize.be/files/ds-field-template_0.png

Status: Needs review » Needs work

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

Bojhan’s picture

Why can't this be a formatting setting of the thing "in general"? If its potentially more, it should clearly not fit in the model shown above.

kclarkson’s picture

It would also be nice to have an option to show random images.

For example you have a node that has 10 images attached, you could have the teaser show one random image from that node.

Right now the random function only works with the "sort" settings for the complete node. So in the teaser I could not have my events sorted by date while also showing the nodes with random images.

Any thoughts?

droplet’s picture

@swentel #7,

I meant if it have 10 images and set to show 2 only. How can I view other 8 images ? There should be a link "View All Images" ?

yched’s picture

@droplet : that's out of scope IMO. But this brings an interesting point in the hairy issue of "read more" links. #823380: Read More link is always visible on teaser.

johnv’s picture

Subscribe

andrewmacpherson’s picture

subscribing, got a keen interest in developing field formatter settings.

@14 + @15:
If a link is out of scope, then how about "and 8 more". It doesn't have to be a link, just an indication that the items being disoplayed are not all there is. Even if it's out of scope for core, I think it's a good use-case for a contrib module. Let's see if we can make it easy to hook into this.

See Field multiple limit and other modules using Field formatter settings API.

droplet’s picture

Ok. I missed the topic.." but you only want to show the first on a teaser view. "

however, I don't see the patch limited settings on teaser view only. If not teaser view exclusively, my comment #14 is important. I hate what drupal does now, some of features 50% in Core, 50% in contrib module.

If only 50% functionally, it needs to rethink again. It's only 30 sites using the contrib module that do same thing.

"Reported installs: 30 sites currently report using this module."

yoroy’s picture

Good feature. I think "Show 1 thumb in the teaser where the full node has 4 images" is a pretty common use case and desirable feature for core. I'm imagining this as an option for each view mode (display mode), where each display mode has the default set to 'show all' (like it is now). Then you see those ugly teasers and start digging into field ui settings…

Hmm. Maybe a core default for the teaser display of the Article content type to limit to 1 would be better then :)

But discoverability of settings for fields is a general problem to be solved on a more fundamental rework of field ui and this is a great little use case to take into account then.

droplet: does a core default to 'show all' on full node already answer your concern?

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
3.92 KB
FAILED: [[SimpleTest]]: [MySQL] 41,734 pass(es), 444 fail(s), and 382 exception(s). View
54.95 KB
48.17 KB

New patch. With #945524: Field formatter settings hooks in core going in, I took a complete different approach where the field module is now the first module in core implementing these hooks which makes sense to me. What do others think of this approach ? Another approach could be that we create a new module inside the field module with field formatter settings that we identify as useful for core.

The patch also moves the settings into the cogwheel. The cogwheel now also shows up nicely when a formatter by default didn't have any settings, e.g. the tags, see attached screenshots. Also, there's a limit and offset now.

1234624-summary.png

1234624-form.png

Status: Needs review » Needs work

The last submitted patch, 1234624-20.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
FAILED: [[SimpleTest]]: [MySQL] 42,116 pass(es), 7 fail(s), and 0 exception(s). View

Urg, stupid variable mix up

Bojhan’s picture

Loosk good to me, kinda sounds like we should have a consistent pattern for this with views going into core, but meh @ consistency. Lets do this.

Status: Needs review » Needs work

The last submitted patch, 1234624-22.patch, failed testing.

andrewmacpherson’s picture

FileSize
3.91 KB
FAILED: [[SimpleTest]]: [MySQL] 42,296 pass(es), 7 fail(s), and 0 exception(s). View

Improving the summary message when displaying all values.
Changed "display_offset all values" to read "Show all values".

andrewmacpherson’s picture

Status: Needs work » Needs review
andrewmacpherson’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,307 pass(es). View

Needs a full stop as well.

andrewmacpherson’s picture

FileSize
3.91 KB
FAILED: [[SimpleTest]]: [MySQL] 42,293 pass(es), 7 fail(s), and 0 exception(s). View

Sorry. uploaded an empty file somehow.

Status: Needs review » Needs work

The last submitted patch, limit-display-fields-1234624-28.patch, failed testing.

andrewmacpherson’s picture

FileSize
4.03 KB
FAILED: [[SimpleTest]]: [MySQL] 42,292 pass(es), 7 fail(s), and 0 exception(s). View

I found a couple of problems:

  1. If the limit was not specified, but an offset was specified, NO items would display (e.g. show all values, skip first value). I have fixed this by carrying out two separate array slices. Skip the offset first, then limit what's left.
  2. If the offset is greater than the number of values actually stored, than no items display (expected), but the field label will still be shown (unexpected).
pcambra’s picture

Status: Needs work » Needs review
swentel’s picture

I'm wondering if we can add an element_validate to this form so that we can cleanup during submit so we can keep having only one array_slice.

Thanks for working on this btw!

The failing tests is because now the two additional keys are added. The main question still might be whether this belongs in field module or in a separate module or not. If we choose for the latter, the field module stays clean and we can possibly add more features to the 'field formatter settings' module. But for that, we need more ideas. Simply just having 'multiple field limit' as a submodule is an option maybe too.

Status: Needs review » Needs work

The last submitted patch, limit-display-fields1234624-30.patch, failed testing.

xjm’s picture

yoroy’s picture

Bump! One of those 'normal' but very jummy feature requests :)

#32 has the to-do's for pushing this further along…

klonos’s picture

...another friendly bump here in order to propose to include a randomizer feature (show x number of values - randomly).

yched’s picture

@yoroy: does it mean you validate the UI ? :-)

swentel’s picture

@yched so do you also validate the approach then ? :) (it's a controversial one I think)

Also, re-looking at the patch again, it doesn't make sense to have a hook in field and one in field_ui, also, the one in field_ui is always going to run, so this one needs work anyway.

yched’s picture

Actually, no, I don't validate the approach ;-). This shouldn't be done by squatting the 'settings' section, which is the domain of each individual formatter type.

But if the UX team is fine with the proposed UI, then I'm fine with getting this in as is, and clearing after Dec 1st.
Because, yes, feature-wise, it would be a very welcome addition.

yched’s picture

Sorry, I realize #39 is unclear - what I mean is :
I'm not fine with the code, those properties shouldn't be stored in the 'settings' section of $instance['display'][$view_mode], but directly at the top level as system-supported, formatter-agnostic properties.

But I'm fine with the UI, and I don't think we can have a better one until the modals patch lands and we can refactor Field UI around modals.

So if the UX team is fine with the UI, +1 on getting this in as is and cleaning the code past freeze.

swentel’s picture

It was perfectly clear, don't worry :) I wasn't actually fond of the approach either. I'll look at the failing tests + adding one for the functionality.

Bojhan’s picture

Issue tags: -Needs usability review

This was already signed off.

swentel’s picture

Status: Needs work » Needs review
FileSize
8.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1234624-43.patch. Unable to apply patch. See the log in the details link for more information. View

Should fix the tests + added tests for limit and offset functionality.

yoroy’s picture

Yup, the UI is what we can do at this stage. It's green, too!

RTBC?

attiks’s picture

This looks good, only found some minor comment issues.

+++ b/core/modules/field/lib/Drupal/field/Tests/DisplayApiTest.phpundefined
@@ -139,6 +139,39 @@ function testFieldViewField() {
+    // Test on limit setting of fields with cardinality not set to 1.

The comment is confusing to me, but might just be me.

+++ b/core/modules/field/lib/Drupal/field/Tests/DisplayApiTest.phpundefined
@@ -139,6 +139,39 @@ function testFieldViewField() {
+    // Limit

THis is a bit short and is missing a trailing period.

andypost’s picture

#43: 1234624-43.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1234624-43.patch, failed testing.

andypost’s picture

Is this possible for D8? patch was nearly RTBC but needs re-roll

andrewmacpherson’s picture

Status: Needs work » Needs review

#43: 1234624-43.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1234624-43.patch, failed testing.

andrewmacpherson’s picture

Issue tags: +SprintWeekend2013

I tried re-rolling this patch over weekend. The reason it no longer applies is that some tests were removed from core. However I couldn't figure out where they should go now, or even if they are still necessary.

As it happens, commits were being made to core all weekend, so I never succeeded in making a patch that applied cleanly.

On the positive side, I managed to port field_multiple_limit.module to 8.x in contrib, and rolled functioning alpha release!

I would still prefer it see it in core though.

johnv’s picture

Testing D8 patches is not (yet) my cup of tea. Someone created a nice D7-module: http://drupal.org/project/vase
A new issue #1968626: There's a patch for that! points to this core issue.

Sweetchuck’s picture

FileSize
60.17 KB

Here is my concept. (very-very early stage)

yched’s picture

Version: 8.x-dev » 9.x-dev

@Sweetchuck: not workable that way. 'settings' is not the place to support generic features supported across all formatters - see #39 / #40.

Also, I'm afraid at this point it's too late for feature requests in the D8 cycle :-/

BrightBold’s picture

Issue summary: View changes

Does this now have the chance to make it into 8.1+ now that the release cycle has changed? It's a good feature.

ivanjaros’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
ivanjaros’s picture

Version: 9.x-dev » 8.x-dev

d8 to trigger tests.

ivanjaros’s picture

Version: 8.x-dev » 9.x-dev

ehm

ivanjaros’s picture

FileSize
3.54 KB

fixed file/image fields

andypost’s picture

Version: 9.x-dev » 8.x-dev

no reason for 9.x this could be done in 8.1

ivanjaros’s picture

FileSize
3.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,558 pass(es), 36 fail(s), and 1 exception(s). View

Class replaced with interface

Status: Needs review » Needs work

The last submitted patch, 61: formatterbase-range3.patch, failed testing.

dawehner’s picture

Nice!

  1. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
    @@ -104,23 +104,88 @@
       /**
    +   * Returns the cardinality setting of the field instance.
    +   */
    +  public function getCardinality() {
    

    Isn't that better as a protected method?

  2. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
    @@ -104,23 +104,88 @@
    +    if ($this->getCardinality() == 0) {
    ...
    +    if ($this->getCardinality() == 0 || (empty($limit) && empty($offset))) {
    ...
    +      if ($this->getCardinality() != 0) {
    

    do we have a magic constant we could use here?

ivanjaros’s picture

As far as I know only unlimited cardinality value has a constant.

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marcvangend’s picture

This issue has been dead a long time - is there a reason why we cannot or should not bring it back to life?

rootwork’s picture

I've been following this issue forever, and I think it would be really useful. @marcvangend if you want to reroll the patch and address #63 I think that would be excellent!

marcvangend’s picture

Version: 8.1.x-dev » 8.2.x-dev

OK, I'll see what I can do. It will have to be re-rolled against 8.2.x if I understand correctly.

rootwork’s picture

It would be a new feature, so yes you're right. Thanks for working on it!

marcvangend’s picture

FileSize
4.29 KB

Here's a re-roll of #61. I changed three things:

1) I made getCardinality() a protected method as per #63.

2) In FormatterBase::settingsForm(), I set $element['limit']['#min'] to 0. It used to be 1, but then you cannot save the form anymore when you want to go back to 0.

3) In FormatterBase::prepareView, the code now proceeds to limit the displayed values if either $offset or $limit is not empty. This (in combination with change 2 above) allows you have an offset without setting a limit, so all values beyond the offset are displayed.

---

For further improvement, I'm curious if we can improve the UX a little. The fields for offset and limit do what they're supposed to, but you cannot really see that they belong together. Maybe we can make it more human readable and compact by turning it into a sentence like this:

DISPLAY ITEMS
Display [____] items, while skipping the first [____].

On the other hand this is not a design pattern that is common in Drupal core. I would love to know what UI/UX people think.

marcvangend’s picture

Status: Needs work » Needs review
marcvangend’s picture

Updated patch, to make sure that all classes that extend FormatterBase::settingsSummary() call the parent method first, before adding their own summary data.

By the way I haven't looked at tests yet, let's see what the testbots think first.

The last submitted patch, 71: 1234624-71-limit-values-to-display.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 73: 1234624-73-limit-values-to-display.patch, failed testing.

marcvangend’s picture

Status: Needs work » Needs review
FileSize
24.75 KB

OK, bear with me, because I'm still learning about config schema definitions. This patch is a bit of a gamble, hoping that it will fix (some of) the test failures. If there are better ways to do the same, please let me know.

Status: Needs review » Needs work

The last submitted patch, 76: 1234624-76-limit-values-to-display.patch, failed testing.

marcvangend’s picture

Yay, down from 829 to 3 fails. To be continued.

rootwork’s picture

Now that's progress! :)

marcvangend’s picture

Status: Needs work » Needs review
FileSize
29.7 KB

See if this comes back green.

dawehner’s picture

Note: It is not required actually to change the base class. There are alternative mechanisms to change output of a field. For example https://github.com/drupal-media/field_formatter/pull/5 provides a wrapper for formatters which does the same.
But yeah this is just some suggestion what could be done instead.

marcvangend’s picture

Thanks for the code example @dawehner. I'm trying to figure out exactly how it would work for a site builder; does it mean that the "limiting formatter" would become a separate formatter that can be selected like other formatters? Or would it automatically affect all available formatters, just like the patch in #80does?

dawehner’s picture

@marcvangend
Yeah I certainly don't say that this patch is not right or anything like that. I just wanted to provide an alternative way.

What this patch would do, is to provide a formatter, which allows you to configure a) limit b) offset and c) the wrapped formatter + formatter settings. So its a bit like formatterception.

marcvangend’s picture

@dawehner Aha, now I see... Not all of the magic is in that PR, part of it is already in the FieldWrapperBase class of the Field Formatter module.

What I like about the wrapper idea, is the separation. After all the limiter is not really a formatter; it is more like a generic preprocessor for any formatter. The wrapper concept allows formatters and "formatter preprocessors" to be mixed and matched. Personally I'm not a huge fan of modules that bundle all kinds of useful helpers in one package (Ctools being the no.1 example), but I guess that's just a matter of personal taste.

Anyway, at this point I would love to get some feedback of Field subsystem maintainers, and ask them what they think of the direction the patch is going, the feasibility of getting this into core, and alternative solutions like the field formatter wrapper.

ivanjaros’s picture

I have simple custom plugin type that also handles this - https://gist.github.com/ivanjaros/d079714f12161d239550f6bb506b35df

and it is being invoked like so:


/**
 * Implements hook_entity_display_build_alter().
 */
function hook_entity_display_build_alter(&$build, $context) {
  /** @var \Drupal\store\Plugin\store\FieldFormatterEnhancer\FieldFormatterEnhancerPluginManager $manager */
  $manager = \Drupal::service('plugin.manager.field_formatter_enhancer');

  /** @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display */
  $display = $context['display'];

  foreach ($display->getComponents() AS $field => $field_information) {
    // Make sure the field if displayed and not empty.
    if (isset($build[$field]) && isset($build[$field]['#items'])) {
      $build[$field] = $manager->viewField(
        $build[$field],
        $build[$field]['#items']->getFieldDefinition(),
        $display->getRenderer($field)
      );
    }
  }
}

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marcvangend’s picture

FileSize
5.09 KB

I created a separate module, based on the code by @dawehner and dependent on the field_formatter module. Hopefully it can help get this issue moving again.

The module works with all core and contrib field types I tested against (from simple text fields to more complex things like paragraphs).

That said, I'm not too happy with some implementation details, especially in FieldLimiter::viewElements(). Currently the limiter formatter simply lets the actual formatter render all values, and then takes out the elements it wants to keep. That's not ideal from a performance point of view. Also, it makes it impossible for the actual formatter to take the number of field values into account. I would rather alter the FieldItemListInterface object and limit the field values before they are passed to the actual formatter, but I don't know if/how that's possible.

marcvangend’s picture

Wait, I think I figured out how to limit the values in the FieldItemListInterface:

    $field_values = $items->getValue();

    $offset = $this->getSetting('offset');
    // To show all elements, expressed by the setting '0', array_slice needs
    // NULL as 3rd argument.
    $limit = $this->getSetting('limit') == 0 ? NULL : $this->getSetting('limit');

    // Let array_slice limit the field values to the ones we want to keep.
    $limited_values = array_slice($field_values, $offset, $limit);
    $items->setValue($limited_values);

I'll try to upload a new version later.

marcvangend’s picture

All right, I created an improved version and created a sandbox project for it: https://www.drupal.org/sandbox/marcvangend/2843702. It's not necessarily my goal to turn it into a full module, I'm open to merging the code with Field Formatter, Field Multiple Limit, or even core. Feedback appreciated.

rootwork’s picture

@marcvangend Message andrewmacpherson, he's the maintainer of Field Multiple Limit for D8. I suspect he would welcome collaborating and/or merging, as the current D8 port has been stuck in a broken state for a little while now.

rootwork’s picture

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

We certainly need some test coverage for the new functionality here.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fgm’s picture

Related issues: +#2864687: Drupal 8 version