Having reviewed the info file, I found:

  • At least PHP 5.4 should be required because of the short array syntax used throughout. The composer file requires webpush ~4 which depends on PHP 7.1 or greater, so the requirement could go up to 7.1, but I don't know of anything in the module itself that requires more than 5.4. It's perfectly reasonable to use short array syntax since webpush needs PHP 7.1 anyway.
  • There is no configure link.
  • Stylesheets should not be included in info files unless they should appear on every page.
  • Since composer_manager is a dependency there's no need to check for its existence in hook_requirements().
CommentFileSizeAuthor
#4 interdiff.txt286 bytesefpapado
#4 3020464-4.patch1.73 KBefpapado
webpush-info.patch1.73 KBjacob.embree

Comments

jacob.embree created an issue. See original summary.

efpapado’s picture

efpapado commented
Status: Needs review » Needs work

Hi, thank you very much for the patch! Before committing it I'd like to clear up two points:

At least PHP 5.4 should be required because of the short array syntax used throughout. The composer file requires webpush ~4 which depends on PHP 7.1 or greater, so the requirement could go up to 7.1, but I don't know of anything in the module itself that requires more than 5.4. It's perfectly reasonable to use short array syntax since webpush needs PHP 7.1 anyway.

You are right on that, but I think that the requirement should be set to 7.1
As you note, Webpush 4 depends on 7.1, and this module does not work without the library, so it should be set to 7.1
On the other hand, maybe the module is compatible with earlier versions of Webpush, so in that case we can change the dependency. If anyone is interested they could test and maybe provide patches.
So for now I'll set it to 7.1 and add to the roadmap testing for earlier Webpush versions.

Stylesheets should not be included in info files unless they should appear on every page.

Maybe I'm wrong, but I think that adding it on the .info file helps the optimisation of aggregation and caching.
I'll try to find more on that.

jacob.embree’s picture

jacob.embree commented

You make good points about the PHP version. I could go either way with it, so I went the conservative way. Let me know if you want me to roll a patch requiring 7.1.

See https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_... for information about drupal_add_css(). Preprocessing is enabled by default. I'm not sure what other differences there are.

efpapado’s picture

efpapado commented
StatusFileSize
new 3020464-4.patch1.73 KB
new interdiff.txt286 bytes

Thanks for the patch and the link. I think that it's better to have PHP7.1 as dependency, since this is what the library needs.

However we can leave it open for future changes, if anyone can provide more info about using older releases of the library.

efpapado’s picture

efpapado commented
Status: Needs work » Fixed

Committed :)

efpapado’s picture

efpapado commented
Status: Fixed » Closed (fixed)