hook_fivestar_widgets() implementations should explicitly include their associated libraries

Thanks for looking into this. The theming part of Fivestar is a mess. The first thing I chose to work on is the templates - right now Fivestar does ALL theming with theme functions, which have been deprecated throughout the life of D8 and are not used in D9. So going forward, that is one big thing we need to address rather soon. The theme functions need to be replaced by preprocess functions and Twig templates. I started that work in #3133125: [Meta] Replace theme functions with Twig templates and I have also made some more progress which I've not yet posted. Additionally, some of the theme functions aren't being used (one has its entire body commented out) and SOME of the theming is being done in the JavaScript now - this is a problem because the markup produced by the JavaScript is *supposed* to be the same as that produced by the theme functions, but it's not and we can't maintain consistency between two parallel theming systems like that. That's an issue I still have to write up and try to address.

The reason I mention all of this is because rewriting the theme functions will necessarily inform me about where and how we should attach assets from contributed modules. I commented on #2984740: Make a proper widget plugin API recently, which makes some good points that you've also touched on above, but I'm thinking that plugins are probably too heavy a solution - all that is being provided by contributed modules are some images and css, no code. The plugin system is probably overkill.

Likewise, I think hook_library_info_build() is probably not the right way to go about it either. It shouldn't be Fivestar's responsibility to define the library with its assets - it should be the responsibility of the module that wants to provide those assets.

I'm starting to get really speculative here because I haven't looked into this at all, it's just an idea at this point. But I've been thinking that a new way to provide widgets will look something like the following:

A contributed module would first implement hook_fivestar_widgets(), same as before. But instead of what we have now in D8:

function mymodule_fivestar_widgets() {
  // Letting fivestar know about my Cool and Awesome Stars.
  $widgets = [
    'path/to/my/awesome/fivestar/css.css' => 'Awesome Stars',
    'path/to/my/cool/fivestar/css.css' => 'Cool Stars',
  ];

  return $widgets;
}

which has a hardwired path (bad, awkward, and probably a security problem because it can pull files from outside web root etc.), I think we should do something like this:

mymodule_fivestar_widgets() {
  // Letting fivestar know about my Cool and Awesome Stars.
  $widgets = [
    'awesome' => [
      library => 'mymodule/mymodule.awesome',
      label => 'Awesome Stars',
    ],
    'cool' => [
      library => 'mymodule/mymodule.cool',
      label => 'Cool Stars',
    ],
  ];

  return $widgets;
}

where mymodule has a mymodule.libraries.yml containing something like this:

mymodule.awesome:
  css:
    component:
      css/awesome.css: {}

mymodule.cool:
  css:
    component:
      css/cool.css: {}

Invoking the hook_fivestar_widgets() will return a list of all contributed widget libraries with a module-defined machine name and a readable label for that library - no need to make up our own way to derive one from the other.

Then we can just use '#attached' => ['library' => [$library]] in our form element or preprocess function when we want to use a particular contributed library of stars.

If we want to go crazy we could even define a hook_fivestar_widgets_alter() function to allow modules to alter the list of widgets to use - for example to handle naming conflicts or sort order etc. (EDIT: I just found #1249398: Add a hook_fivestar_widgets_alter() to allow modules to remove or change the star display styles which also proposes this alter function, so let's just do this - it's not so crazy after all.)

I think this addresses all your points in the original post, plus it does away with Fivestar dealing with paths everywhere and decouples Fivestar from the module providing the widgets. And I think it allows us to address #1249410: Simplify the list of star display options as well.

So in summary, a contributed module that wants to provide star just needs to:

1. Implement hook_fivestar_widgets()
2. Define a library for the css provided

Fivestar would discover the contributed stars using hook_fivestar_widgets(), allow alteration via hook_fivestar_widgets_alter(), and include the correct library via #attached where appropriate.

What do you think?

Oh, and I should mention that while my way does constitute an API change (because the return value of hook_fivestar_widgets() has changed), we are in alpha still and the original post demonstrates that nobody is yet providing additional widgets for the D8 version yet - at least not successfully, as this feature doesn't work. Thus, it's the perfect time to make an API change. I think if we do this then #7 can be put into the documentation guide as an explanation of how to do this, plus we should have a test module included with fivestar which provides widgets as an example and a test case which uses this test module and makes sure provided widgets can be discovered and used.

That would be great, just post another patch when you have something that works for you, I'm sure you'll have to make some adjustments... You have the perfect use case since you're actually trying to provide widgets with your own code.

The tests should be easy once Fivestar supports this feature - I can write those after seeing your patch.

That was quick! I'll look at it tonight. Just a quick question for now - you say:

    'cool' => [
      'libraries' => [
        'some_other_module/cool_dependency',
        'mymodule/cool',
      ],
      'label' => 'Cool Stars',
    ],

But do we really need to do this? Can't we do this in mymodule.libraries.yml like this? :

cool:
  css:
    component:
      css/cool.css: {}
      css/even-more-cool.css: {}

  dependencies:
    - some_other_module/cool_dependency

plus mymodule could also add some JavaScript like this if desired ...

I didn't really have time to look at this very much. Overall it looks like it's doing things the right way. There are a few changes I'd make to the patch:

Remove the changes to includes/fivestar.theme.inc, as those are just coding standards and out of scope for this issue (besides, that is all going to disappear once the theme functions are moved to templates).

fivestar_get_widgets(), fivestar_get_widget_info(), fivestar_get_widget_label(), fivestar_get_widget_libraries() should all be in a service. (You mentioned that, and I agree). Maybe fivestar.widget_manager implemented by src/WidgetManager.php

+   * Sites that used an older version of the module will have
+   * a stale key set for their selected widget. This returns
+   * the proper, cleaned up version if that's the case.

This probably should be in a hook_update_N() so that it is only done once, not every time on every page forever.

OK, I applied and tried out the patch. Here's a new patch based on my experience, with an interdiff.

Things I altered

In the code in fivestar_fivestar_widgets() where you define the widgets provided by the Fivestar module you say:

// There's probably a way to pick these up from the filesystem.

And there is:
$libraries = \Drupal::service('library.discovery')->getLibrariesByExtension('fivestar');
The keys of the $libraries array will be the machine names of the libraries defined in fivestar.libraries.yml. There are two problems in doing this:

1. We would need a way to separate the widget library definitions from the other library definitions in fivestar.libraries.yml. Perhaps a naming convention that all widget libraries are prefixed by 'widgets_', but naming conventions are a poor solution.
2. Even if we pick up the widgets automatically, we still need a way/place to specify their human-readable labels. You currently do this with ucfirst(), but that's also a poor solution because it gives us no discretion in naming.

For these two reasons I feel that the best option for now is to explicitly give the widget machine name and human-readable label directly in hook_fivestar_widgets() without trying to automate it. I have an idea of how to do this better, but that will have to wait because it takes us on a big detour from what we're doing here in this patch. For now, let's just hardwire the labels to remain the same as before (which is also the same as ucfirst gives us ...) but after this current issue is finished we can implement #1249398: Add a hook_fivestar_widgets_alter() to allow modules to remove or change the star display styles without a problem this way.

Next, Fivestar's implementation of hook_fivestar_widgets() should look the same as any other widget-providing module. We don't need to do anything special here. In particular, since getWidgets() already statically caches ALL the widgets, we don't have to have another static cache just for the widgets defined by this main module. This simplifies the hook a lot.

I also added a test module called 'fivestar_widget_provider' which just provides the two widgets mentioned above "Awesome Stars" and "Cool Stars". This serves as a demonstration of how to write a module that provides widgets, but more importantly gives us a way to test the operation of hook_fivestar_widgets(). I haven't written the test yet to use this test module to test the hook, but that's coming ...

I moved the methods you put into VoteManager into a new WidgetManager service because they have nothing to do with votes.

Some still-outstanding problems

drupal_static() vs \Drupal::cache():
drupal_static() is per-page-load. \Drupal::cache() caches the data in the database. The latter is not what we want, since then there will be no way to detect new or removed widgets. It's possible to use the database cache, but we would need to add cache tags and invalidate those tags etc, and in the end I don't think it will result in any gain. The per-page-load drupal_static() cache is sufficient - we do one discovery of widgets per-page-load, and since hook_fivestar_widgets() should return a fixed array from each module, that doesn't even require a database query or a file system scan. It's super fast already. One could even argue that the drupal_static() is overkill, but it doesn't do any harm either.

I'm not a fan of @return string|null, @return array|null, etc. Those are ancient PHP-isms. We should try to be as type-definite as possible in all new code, that way we don't need to be testing for isset() and !empty() all over the place. So specifically if the function returns a string it should always return a string. If the widget has no label, that string should be '' rather than NULL. Likewise, return [] instead of NULL for functions that return arrays.

You injected a service into StarsFormatter but that should really go into FivestarFormatterBase so that the injected service can be shared by all formatters. Also, while you changed the FivestarWidgetBase to use the new widget discovery and to remove getAllWidgets(), you didn't change the FivestarFormatterBase in the same manner - both FieldWidgets and FieldFormatters need access to the Fivestar widgets.

I'll take a look into writing the hook_update_N() in addition to the tests I mentioned above.

I forgot to change some of the voteManager usages to widgetManager in the patch, that's why it failed. That's fixed in this new patch.

In the meantime, I wrote a test for hook_fivestar_widgets() to verify that the widget libraries provided by the new 'fivestar_widget_provider' test module can be discovered and used. This new test is include in this patch.

There's a lot of duplication of code between the FieldFormatters and FieldWidgets right now. I don't think we should try to fix that in this issue, but yes we should fix that at some point. (We're not talking about stars widgets in this post, we're talking about field widgets. We might want to choose a different term for fivestar star 'widgets' at some point to reduce confusion. Maybe glyph or sprite or something like that, but most of these alternatives will also come with baggage ...)

For most fields, there is a big distinction between the formatter, which is used to display the field value, and the widget, which is used to set the field value. Widgets are normally used in an admin capacity, when creating a node with a specific field value for instance, while formatters are used when viewing the field value when viewing the node. This is basically the MVC (Model-View-Controller) pattern, where the formatter is the View and the widget is the Controller.

With Fivestar, we don't have that clear separation because the stars can be set/chosen (which is normally the job of the widget) when viewing the content (viewing would normally be the job of the formatter). That's how rating works, which is different than how most fields work. This is a variant on the MVC pattern which I normally call Model-Delegate, where the Delegate is a combination of the View and Controller. Drupal core does not directly support this pattern.

So because in Fivestar we must do some of the same things in the widgets and the formatters, that means duplicate code. Eventually we should abstract that out so they share some common base class or trait or something. But for now, it's important to make code changes in both the formatters and the widgets.

Give me another day to look at this - I'm in the middle of a couple of other things right now ...

Sorry it took so long. This patch is getting pretty complex and with all the code movement and the other things I've been doing it took me a while to check how this differed from #22.

Here's a new patch, the only difference from #28 is some coding standards - @param comments and that sort of thing. I think this is now ready to commit with the exception of the update path.

... thinking out loud about the update path ...
hook_update_N() is only important if we're changing the way content is stored in the database, whether it's the schema of the data or the data itself. What we're changing in this patch is the format of the data that's returned by the hook_fivestar_widgets(), and specifically the key that identifies the widget selected. The only place this is stored is in the entity configuration for the rated entity.

So for example, I have a field "fivestar_rating" on my content type "article". There are three place where the widget 'key' is stored, namely in the three configuration entities for the article view modes:

core.entity_form_display.node.article.default
core.entity_view_display.node.article.default
core.entity_view_display.node.article.teaser

This makes sense to me - each entity type (in this case an article, which is a core node type) may have its own fields, and each of those fields may have its own settings, and each view mode may have its own field settings (whether to display that field or not in the view mode, for example). So really the only proper place to store a "key" telling us which widget is being used is in the entity view mode configuration data.

That makes it really difficult to find and fix the old-style keys which look like "modules/fivestar/widgets/basic/basic.css" and change them into the new-style keys which look like "basic". We would have to query every view mode of every entity searching for a "fivestar_stars" type field, then change the "fivestar_widget" settings value for each one. Or maybe just query every instance of each FieldWidget we define. Regardless, it's going to be very time-consuming to develop a hook_update_N() hook to do that correctly, along with a test for that update.

... end thinking out loud...
So what you're doing in StarsFormatter::getWidgetSetting() and FivestarWidgetBase::getWidgetSetting() is to identify any "old" keys and try to derive the "new" key name from that old key name. That's nice for BC, but we don't want to do it forever - the weaknesses of this approach is 1) it doesn't change the data in the database so that it needs to correct this old key on every page, and 2) it's really only a guess as to what the new key should be. A very good guess, that works in all known cases, but still a guess because the new key doesn't necessarily have anything in common with the file name of the .css file.

(and BTW shouldn't it be FormatterBase::getWidgetSetting() instead of StarsFormatter::getWidgetSetting() ?) so that the widgets and formatters do the same thing?)

I don't have a better idea of how to do this. I guess I would leave in your BC layer for now, forget about the hook_update_N(), but maybe put in a @todo that this should be removed at some future time? At some point sites that currently have Fivestar installed the update to this new patch are going to have their site break - is there something we can do to prevent that or ease that transition? We ARE in alpha still, so we're allowed to skip the update path, but I don't want to do that unless it's too much work. It's far more important to me to get a stable full-featured release, so I'd rather spend the effort on that than on an update path from one alpha to another alpha.

I forgot the --binary on the patch so the new images didn't come through. I cancelled the test in #29. Here's the new patch with the images.

(Yes, I know this has 25 new coding standards problems, but 22 of those are because of the new CSS files for the test widgets - these should be addressed by #3137467: Fix CSS coding standards when we figure out how to re-write our "standard" widget CSS so that it is free from coding standards violations. The remaining 3 are false positives, which I have no desire to spend effort on avoiding.)

OK, here's what I want to do. I want to implement a "best effort" update function, meaning let's try our best to minimize disruption by trying to update the old data in the database, and provide instructions on how to proceed if our best effort doesn't work. The way I want to do this is basically the same as I described in #29.

Attached is a file named fivestar_update_8101.txt that contains a prototype update function.

You may download this file then run it with drush, using the command drush php-script fivestar_update_8101.txt
You must install the patch from #30 first. otherwise this drush command won't work.

This will perform a DRY RUN of the update. It will NOT modify any of the data in your database (that part is commented out), it will just tell you which 'legacy' widget names you are using and what it proposes to rename them to. This should give the correct answer for ALL stars widgets defined by Fivestar, but may fail gracefully on stars widgets defined by other modules.

There is a lot of text in there explaining what I've done.

PLEASE TRY THIS ON YOUR SITE. You should have a site that has not been patched with #30 yet. Patch it, download this drush command, then see if it correctly identifies all your Fivestar fields and which stars widgets they use, and then see if the proposed rename is correct.

If this works as expect I will want to remove the BC layer from StarsFormatter::getWidgetSetting() and FivestarWidgetBase::getWidgetSetting(), then add the hook_update_N() (with the rename code uncommented), and roll a new patch to be committed.

I do NOT intend to write an update test - this is an alpha-to-alpha update with no expectation that the update will be non-breaking. I think this is a great try at avoiding problems, but I'm not willing to do much more work on this.

BTW, I did consider your 'legacy_key' idea and I think it's good and will work. But it will only work for star widgets provided by Fivestar, all others will fail. And we would now have 'legacy_key' information passed around going forward - it would still need to be removed at some point.

My code above is a one-shot deal which will also work for all Fivestar star widgets but will not add any code that needs to be removed later. It will also probably work for most contributed widgets, and if it doesn't at least it will let you know how to fix the ones that failed. Plus it was a little more fun to write it this way ...

And here's a hack - if you have a site that has already applied #30, you can edit fivestar_update_8101.txt and change "NOT IN" to "IN" - then running this will list your Fivestar fields that are using the NEW hook_fivestar_widgets() output. I have used this to verify that the update function works with multiple Fivestar fields, on multiple content types, and with different widget settings on different view modes.

Thanks for the amazing work and interesting discussion.

Here some minor points/questions which I found during code review. Later I'll try manually to test the code and upgrade path.

1. +++ b/fivestar.module
   @@ -196,50 +195,54 @@ function fivestar_form_field_ui_field_edit_form_alter(&$form, $form_state) {
   +      'label' => 'Basic',
   

   Should we use t() here?

2. +++ b/src/Plugin/Field/FieldFormatter/StarsFormatter.php
   @@ -80,10 +80,8 @@ class StarsFormatter extends FivestarFormatterBase {
   +      '@widget' => ($label = $this->widgetManager->getWidgetLabel($this->getWidgetSetting())) ? $this->t($label) : $this->t('default'),
   

   How about adding the second optional parameter into getWidgetLabel which allows passing default label?

   Also pass a variable into t() looks wrong.

3. +++ b/src/Plugin/Field/FieldWidget/StarsWidget.php
   @@ -51,10 +52,8 @@ class StarsWidget extends FivestarWidgetBase {
   +      '@widget' => ($label = $this->widgetManager->getWidgetLabel($this->getWidgetSetting())) ? $this->t($label) : $this->t('default'),
   

   The same here.

4. +++ b/src/WidgetManager.php
   @@ -0,0 +1,113 @@
   +  public function getWidgetInfo($target_widget_key) {
   

   In another widget manager functions, we use $widget_key. I think here we also should use it instead of $target_widget_key

5. +++ b/src/WidgetManager.php
   @@ -0,0 +1,113 @@
   +    foreach ($this->getWidgets() as $widget_key => $widget_info) {
   

   We can make the code a bit simpler:

   
   $wigets_info = $this->getWidgets();
   return isset($wigets_info[$widget_key]) ? $wigets_info[$widget_key] : [];
   

1) Yes. Need to make this change in the test module too.
2) It looks a little wrong, and this sort of use is discouraged, and my gut instinct when I saw that was just to remove it, but it's actually the correct thing to do here once 1) is done. When we do t('Basic'), that adds that label to the translation tables, then t($label) will look up the correct translation in the table. The reason we don't use arbitrary variables in t() is because the whole t() mechanism doesn't work unless we can guarantee that the string we're trying to translate will be present in the translation table. Any $label we use here WILL be defined in a hook_fivestar_widgets() so it will always be available for translating, provided 1) is done.
3) I don't think an extra parameter is the right way, but I think that there SHOULD be a default widget setting that get used. Why isn't getWidgetSetting() returning 'default' if there is no widget chosen yet? It appears that the default setting of 'fivestar_widget' is 'basic' - is this appropriate or maybe this should be 'default'? I don't know how 'basic' and 'default' were being used in D7, but I have noticed in doing other things that these aren't handled properly in D8 yet. The use of isset() or !empty() in OO code to me almost always means that a data type hasn't been initialized properly, so to me the fix should be to make sure it gets a proper default value to begin with rather than testing if it's been set and assigning a value if it hasn't been set.
4) Yes.
5) Yes, we don't need the loop.

There is so much going on here that I don't want to keep re-rolling this patch. The last re-roll took me days to work through, so I'm hoping for at most one more re-roll with an interdiff. I think we can do all of the above, then maybe handle any other problems in a follow-up issue where any changes can be more manageable? I don't think the current patch is perfect, and I know there are other things I want to do eventually like I mentioned in #25, so I want to get over the hurdle of changing the hook first then we can make other improvements. See for examp[le #1249410: Simplify the list of star display options.

Here is update according to points in #37. It also contains some other small corrections.

'basic' vs 'default' - yes this moment certainly requires some clarification.

I looked over the interdiff and all the changes look good to me. I'd like to play with this on my test site for a couple of days just to make sure everything works, but I think this is ready to commit unless some problem arises (which I do not expect, but it never hurts to test more ...)

What do you think @mrweiner?

#40 failed to apply because --binary was not used when making the patch (this patch contains binary image files).

Here is a re-roll of #40 for testing.

Besides the --binary, I also include the hook_update_N() function for this change, which is what I posted in #34 with less verbose output. I have tested the update function manually on a number of sites. Because it is a non-destructive update, you can run it again and again if you like. Just reset the schema version in your DB ( drush php:eval 'print_r(\Drupal::keyValue("system.schema")->set("fivestar", 8000));' ) and then you can run the update again.

Testing #44 against D9.0 shows there are a few test asserts that have to be changed because DrupalCI uses PHPUnit 8.5 for D9.0 tests but only uses PHPUnit 6.5 for D8.8 tests. A number of assert functions have been removed or deprecated in the newer PHPUnit.

Sorry about #40 I didn't notice moment with files.

For 'basic' vs 'default' I just have created: #3149852: Correctly determine default widget

Also, I've tested the update on my test site and everything is ok.
I only noticed one minor moment - we show a message with replacing statistics only for entity form displays.
It's easy to fix by moving $message above foreach (['entity_view_display', 'entity_form_display'] as $display) {
but in that case, output not ideal(screenshot attached).

Maybe it will be better not to show that message?

@init90: Yes, I initialized $message in the wrong place.

@mrweiner: The Drupal UI shows the message if you run the update on /update.php. But if you do it in drush using drush updatedb then the messages are ignored by drush and not displayed.

I guess for that last reason it makes sense to just forget about the messages - if you're running the update from the UI then it's less likely you'll find the messages meaningful, and besides most core update functions don't show messages so it's really not something that's expected.

I'm going to commit this now with the message removed. If there are any other issues with the patch they are probably minor at this point in time after all this scrutiny, so let's just handle anything else in a new issue.