With hook_fivestar_widgets(), you can add different star display styles to the available list. But you can't remove or rename the existing ones.

This patch adds a hook_fivestar_widgets_alter() to allow other modules to completely control the types of stars that users can choose from.

CommentFileSizeAuthor
#18 1249398-18-hook-fivestar-widgets-alter.patch5.31 KBTR
#17 interdiff-10-17.txt2.49 KBTR
#17 1249398-17-hook-fivestar-widgets-alter.patch5.3 KBTR
#13 interdiff_10-13.txt448 bytespavnish
#13 1249398-13.patch2.77 KBpavnish
#10 1249398-10-hook-fivestar-widgets-alter.patch2.76 KBTR
hook-fivestar-widgets-alter.patch3.96 KBDavid_Rothstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Oh, I forgot to mention: This patch is rolled on top of the patch at #547050: Widget per axis (it will only apply after that one is committed).

I did that because it looks like that will be committed soon, and it adds a bunch of module_invoke_all('fivestar_widgets') calls and such so it totally changes the code that would be necessary here.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Hm, reviewing my own patch... it looks like it will definitely allow options to be removed, but things get complicated when renaming options, since the module uses strtolower() in some places to go between the human-readable and machine-readable name.

That may also mean that the patch's inclusion of t('Default') in hook_fivestar_widgets() is problematic too (will cause problems in other languages).

I think to fix this properly and make it fully alterable we'd actually need to clean up the associated code and change the array structure of hook_fivestar_widgets() itself so it separates the human and machine names, something like:

$widgets['machine_name'] = array(
  'name' => t('Human readable name'),
  'css' => ....
);

Would that make sense?

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs review » Needs work

Makes sense indeed. Uhm I'm ok with rolling it with this patch, or we can open a separate issue.

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Priority: Normal » Major

Moving this up.

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Issue tags: +fivestar-alpha-2-blocker

tagging

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol commented

#2 would be nice. I tried to use "Big Stars" and the css ended up like "foo.big stars".

whiteph’s picture

whiteph CreditAttribution: whiteph commented
Priority: Major » Normal
Issue summary: View changes

Moved the priority down. Using the definitions in Priority levels of issues, where it says Tasks and Feature Requests are very rarely “critical”. They should usually be “normal” or “minor”.

The patch really does need re-work as it no longer successfully applies, even with a 3-way merge. And there's still the question of the "Default".

TR’s picture

TR CreditAttribution: TR commented
Version: 7.x-2.x-dev » 8.x-1.x-dev
Component: Code » Widgets
Issue tags: -fivestar-alpha-2-blocker

Not in D7 at this point in time, but I think this is a good idea for D8.

TR’s picture

TR CreditAttribution: TR commented
Status: Needs work » Active

This should be easy to do now that #3136366: hook_fivestar_widgets() implementations should explicitly include their associated libraries has been committed.

TR’s picture

TR CreditAttribution: TR commented
Status: Active » Needs review
FileSize
1249398-10-hook-fivestar-widgets-alter.patch2.76 KB

This patch adds hook_fivestar_widgets_alter() for D8 and provides a test implementation and a test case.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented
Status: Needs review » Needs work

Looks totally good! One thing that left is documentation about the hook in fivestar.api.php

pavnish’s picture

pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented

+ protected function setUp(): void {

setUp function should type hunt with void.

pavnish’s picture

pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
1249398-13.patch2.77 KB
interdiff_10-13.txt448 bytes

Please review this patch for #12

pavnish’s picture

pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs review » Needs work

#11 still need to be work

TR’s picture

TR CreditAttribution: TR commented

@pavnish: No, we're not adding return type declarations or static type hinting now, certainly not as part of this issue. If you want to tackle just the return type declarations for the ENTIRE Fivestar code base then please open a new issue and supply a patch - when this is done it will all be done together.

#3050720: [Meta] Implement strict typing in existing code

pavnish’s picture

pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented

@TR Thanks for suggestion we will create a new issue for that once this is done .As of now Please ignore #12 #13

TR’s picture

TR CreditAttribution: TR commented
Status: Needs work » Needs review
FileSize
1249398-17-hook-fivestar-widgets-alter.patch5.3 KB
interdiff-10-17.txt2.49 KB

Here is patch #10 with the additions to fivestar.api.php mentioned in #11.

TR’s picture

TR CreditAttribution: TR commented
FileSize
1249398-18-hook-fivestar-widgets-alter.patch5.31 KB

Let's see if we can get rid of the CS message.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented
Status: Needs review » Reviewed & tested by the community

Thanks! Looks good for me.

  • TR committed 7f99522 on 8.x-1.x 
    Issue #1249398 by TR, David_Rothstein, init90: Add a...
TR’s picture

TR CreditAttribution: TR commented
Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

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