Hello team,
I recently used views to build my search page, and added a Global Result summary block in the header. I see few tokens are available. I used the token "@total" along with I need to display "item" OR "items" based on number of results. I could not find any way of doing it via the UI.
I had to do alter and make the text plural.
I was told that, tokens can be help here.
Can you please look into this tiny thing, where it support plural, and also support its translation.
Happy to provide any further info and help.
Thanks.
Comment | File | Size | Author |
---|---|---|---|
#90 | 2888320-90.patch | 19.41 KB | Henry Tran |
Issue fork drupal-2888320
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
tamarpe CreditAttribution: tamarpe commentedBased on
https://www.drupal.org/node/1793500
With the patch you be able to adjust it in the view settings as admin.
Comment #4
tamarpe CreditAttribution: tamarpe commentedComment #5
fcastand CreditAttribution: fcastand commentedComment #6
fcastand CreditAttribution: fcastand commentedHello team,
I needed to have plural on my results count in Views for 8.3.7 version of Drupal.
I had an error when applying the patch for 8.4 version , so I adapted it for 8.3.7.
Comment #7
fcastand CreditAttribution: fcastand commentedComment #8
julbo CreditAttribution: julbo at Smile commentedHi !
With the patch in https://www.drupal.org/node/2888320#comment-12303083 when there is 0 result it is in plural whereas it has to be singular IMO.
So here a new patch for Drupal 8.3.x with an interdiff.
We have the same issue with the patch in https://www.drupal.org/node/2888320#comment-12226674 so I'll update it sooner.
Comment #9
julbo CreditAttribution: julbo at Smile commentedThe is the new patch with the fix (https://www.drupal.org/node/2888320#comment-12340067) in 8.4.x version with the interdiff.
Thanks
Comment #10
scuba_fly+1 RTBC Works for me.
Also 0 gives the plural 'results'
Comment #11
LendudeSeems like a feature request more then a task. New features go into the development branch, 8.5.x at the moment.
Some feedback on the current patch:
all arrays need to be updated to the short [] notation
Would be nice to have some #states on these fields and only show them when 'format_plural' is checked
This comment is not very helpful
We'd need test coverage for this and an upgrade path for the new settings.
Comment #13
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com at Deloitte Digital for Deloitte Digital commentedFixed the following feedback given by @Lendude.
1 - Fixed all the arrays with short [] notation.
2 - Added drupal #states and only show relevant field when 'format_plural' is checked.
3 - Update the comment.
Comment #14
stephenrodrigo@yahoo.com CreditAttribution: stephenrodrigo@yahoo.com at Deloitte Digital for Deloitte Digital commentedComment #15
DakwamineDoes the patch support translation? I couldn't find any field to configure the plural translation.
Comment #16
DakwamineI have added to views.area.schema.yml the plural field in order to get this same field translatable.
I removed a value (
translatable => TRUE
) from the defineOptions method which does not have any impact on the translatability of the field. If I am wrong, you can restore this line (check the interdiff for details).I added a line break to a comment to respect code standard (phpcs).
Comment #18
lfilipov CreditAttribution: lfilipov commentedWe had a project where we had to show different messages for plural forms but we also had to cover the case where there are *0 results*. This patch did the trick for me.
Comment #19
andypostShould be
$this->t()
Comment #20
andypostNeeds work for tests, upgrade path & naming
Bad naming, suggestion is "format_plural_text"
This options also needs config schema
Comment #21
MattDanger CreditAttribution: MattDanger commentedThis patch worked great for me. I've attached a slightly updated version with feedback from andypost.
Comment #22
Jim.M CreditAttribution: Jim.M commentedAttaching an updated patch of the one posted in #21 by MattDanger.
Every language has a different pluralization formula. It's not enough to just check if the number is more than 1 to decide which variant to show. Even for English this approach fails for zero results. In order to use the formulas, the updated patch uses
PluralTranslatableMarkup::createFromTranslatedString()
, as the view gets translated with the config translation.Comment #23
davidferlay CreditAttribution: davidferlay at Skilld commentedHeader UI in view
Single/plural string translation
Comment #24
davidferlay CreditAttribution: davidferlay at Skilld commentedComment #25
alexpott@davidferlay thanks for the manual tests but we also need automated test coverage of new features. For more information about writing tests in Drupal 8 see the following links:
Also as per @Lendude we need an upgrade path.
Comment #27
dorian.dirusso CreditAttribution: dorian.dirusso at Smile commentedHello,
Here is a patch to provide automated tests. I have been helped by https://www.drupal.org/u/grimreaper.
But I don't find any existing test to guide me to test that the plugin is translatable.
I will try to search for an upgrade path.
Thanks for any help on the upgrade path and the translation tests.
Comment #29
dorian.dirusso CreditAttribution: dorian.dirusso at Smile commentedHello,
We tried to fix problems with test and we found a solution for an upgrade path
I put this in this new patch.
Thanks for the review.
Comment #31
dorian.dirusso CreditAttribution: dorian.dirusso at Smile commentedSorry,
I forgot to insert the original modifications. I re-upload the patch with all the changes.
Comment #32
blake.thompson CreditAttribution: blake.thompson at Slalom commentedI've tested the patch in #31 and it appears to work for plural formatting, but there doesn't seem to be any support for non result summary tokens. The issue title seems to imply that there was a desire to support other tokens but maybe I'm misunderstanding. Is there any reason this views area shouldn't be extending TokenizeAreaPluginBase instead and calling tokenizeValue() on the output?
Comment #33
blake.thompson CreditAttribution: blake.thompson at Slalom commentedAdding patch with token support in case it's useful.
Comment #35
dorian.dirusso CreditAttribution: dorian.dirusso at Smile commentedHello,
I have test your code and its works, but you have forgotten the views test. I add it. The test works now.
Thanks for the review.
Comment #36
dorian.dirusso CreditAttribution: dorian.dirusso at Smile commentedComment #39
GrimreaperComment #40
GrimreaperHello,
Here is an updated patch from comment 35.
I found that it was missing "tokenize" key in the config has now the Result plugin extends TokenizeAreaPluginBase.
Comment #42
GrimreaperComment #43
GrimreaperFixing schema.yml
Comment #45
GrimreaperComment #46
GrimreaperFixed Unit tests.
Comment #47
GrimreaperTests are green \o/!!! Finally...
Comment #48
LendudeI think 'special handling' is too vague, can we get a clearer description of what this does?
This could be clearer too, reading this it's not immediately obvious to me how to use this.
Can we make this a post_update function and use ConfigEntityUpdater? See \views_post_update_limit_operator_defaults
Also this still needs an update test.
Comment #49
GrimreaperComment #50
GrimreaperComment 48 taken into account.
Unfortunately I have the update test failing locally, the new update hook is not detected. I will see with testbot if it comes from my setup.
Also catched that the tokenize key was not inited with the previous update hook.
Comment #51
GrimreaperRebasing patch.
Conflict in the views.post_update.php file.
Comment #53
GrimreaperComment #55
GrimreaperAnd here is a new patch.
Not putting an interdiff because of the new strategy decided with Lendude => Creating a new dedicated plugin.
So it is a completely new patch.
Maybe the issue title should be changed.
Comment #56
LendudeJust to explain the reasoning behind the new plugin approach:
- way less disruptive
- no update paths needed
- Much much much clearer UI (the modal for the previous patches was pretty hard to read)
Will try to do a proper review soon
Comment #57
GrimreaperComment #58
rachel_norfolkretagging
Comment #59
LendudeThis looks great. Some nits:
This needs to explain what it does not why it exists :)
Both test methods have the same doc bloc, maybe specify which part they are testing
Can we give a better explanation here then just (Plural handled)?
"Shows result summary, for example the items per page, with support for having singular and plural text depending on the number of results."
I don't know, something like that? Just to make it clearer what the difference is between the two plugins.
Also, can we get some fresh screenshot of the new plugin, the ones in #23 are no longer relevant but they provide a nice arguments for the new plugin when combined with the new screenshots I feel.
Comment #60
GrimreaperHello,
Thanks @Lendude for the review :)
1: Done, hope it is ok.
2: Done
3: Done, your sentence is better, I just didn't know that it was ok to have long sentence.
And screenshots attached.
Comment #61
idebr CreditAttribution: idebr at iO commentedIt would be nice if the default value for the plural actually used a different text to showcase the difference with the current plugin.
Suggestion singular form:
Displaying 1 result.
Suggestion plural form:
Displaying @count results.
Comment #62
GrimreaperThanks @idebr for the review.
Marking as RTBC to get feedback from maintainer before implementing your suggestion.
Comment #64
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedRe-roll patch #60
Comment #66
Clauce CreditAttribution: Clauce commentedThanks for this great work. I have been using this patch for a long time now.
But sadly, it's not working anymore since 8.8.4 Drupal core, neither with 8.9.x.
Comment #67
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixing failed tests.
Comment #69
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedComment #71
GrimreaperHello,
Why making this change? (comment 67)
Comment #72
GrimreaperHello,
Ok I got it.
PluralTranslatableMarkup::DELIMITER
was deprecated.As there is no interdiff in comment 64, I tried to apply patch from comment 60 on 9.2.x which still applies, so I don't understand the comment from patch 64.
And I confirm that I obtain the same code as patch from comment 59. after updating the deprecated code.
So back to RTBC to have maintainers feedbacks.
Comment #73
alexpottThis is super tricky but this is not how we should be doing this. The problem is that this needs to be translatable so we need to be mindful of multiple plural forms for different languages. See \Drupal\views\Plugin\views\field\NumericField::buildOptionsForm() for an example of the work we need to do here to support multiple plural forms.
Comment #75
PhilYWith no opinion on the cleanliness of the code, patch #69 seems to be working on a Drupal 9.2.2 website using English & French languages (translated plural field follows language as expected).
Comment #77
MatroskeenAttaching a re-roll of #69 for 9.3.x. Still needs work for #73
Comment #78
MatroskeenAha! The use of
Drupal\views\Plugin\views\area\Xss
was removed in #2569381: Drupal\views\Plugin\views\area\Result does an unnecessary XSS::adminFilter() . Now we have to restore it here.Uploading a new patch for 9.3.x.
Comment #80
chOP CreditAttribution: chOP at Deloitte Digital commentedHave been using this patch at #78 for a while.
Would like to see addition of one more token that will number format the "total" token with a thousands separator.
Could be something like: @format_total
with a replacement like:
Would that be okay to add within this issue and proposed patch?
Comment #82
chOP CreditAttribution: chOP at Deloitte Digital commentedThe merge request at #81 applies patch #78 with the following additions
Comment #83
chOP CreditAttribution: chOP at Deloitte Digital commentedtest failures picked up problems with the naming of the new token and the formatting call.
Fixed those now, rebased and queued for testing again.
Comment #84
chOP CreditAttribution: chOP at Deloitte Digital commentedSome thoughts:
Comment #85
PhilYedited wrong post
Comment #86
Jelle_SRE #78: it was removed for a reason, so it shouldn't be restored I think, but removed from this patch/MR as well?
Comment #89
Henry Tran CreditAttribution: Henry Tran at Deloitte Digital commentedWe are still using the patch, so I re-rolled the patch to Drupal 10.1
Comment #90
Henry Tran CreditAttribution: Henry Tran at Deloitte Digital commentedComment #91
PhilYPatch #90 works for me using Drupal 10.1.5