The Flexible weather API module provides the ability for adding weather blocks to your cite. This module based on the Drupal plugin system and has a flexible way of expanding.

Currently the module includes three weather services:

  • OpenWeatherMap
  • WeatherStack
  • YahooWeather

Project link

https://www.drupal.org/project/flexible_weather_api

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/flexible_weather_api.git

Comments

alexander.levitsky created an issue. See original summary.

alexander.levitsky’s picture

alexander.levitsky commented
Issue summary: View changes
shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
Status: Needs review » Needs work

Hi @alexander.levitsky,

Thanks for your contribution. Please fix coding standard issues.

FILE: .../weather_api_plugin/src/Plugin/WeatherApiPlugin/WeatherStack.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 63 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 63 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
 73 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 73 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../weather_api_plugin/src/Plugin/WeatherApiPlugin/YahooWeather.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 85 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 85 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
 95 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 95 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...eather_api_plugin/src/Plugin/WeatherApiPlugin/OpenWeatherMap.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 62 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 62 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
 72 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 72 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...i/modules/weather_api_plugin/src/Annotation/WeatherApiPlugin.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 38 | ERROR | Class property $base_url should use lowerCamel naming
    |       | without underscores
 54 | ERROR | Class property $overridden_label should use lowerCamel
    |       | naming without underscores
----------------------------------------------------------------------


FILE: ...ther_api/modules/weather_api_plugin/src/WeatherApiPluginBase.php
----------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 54 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 54 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
 61 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 61 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
 68 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 68 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...r_api/modules/weather_api_entity/src/Form/WeatherServiceForm.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 232 | ERROR | [ ] Doc comment short description must start with a
     |       |     capital letter
 232 | ERROR | [x] Doc comment short description must end with a full
     |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...her_api/modules/weather_api_entity/src/Entity/WeatherService.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 119 | ERROR | [ ] Doc comment short description must start with a
     |       |     capital letter
 119 | ERROR | [x] Doc comment short description must end with a full
     |       |     stop
 146 | ERROR | [ ] Doc comment short description must start with a
     |       |     capital letter
 146 | ERROR | [x] Doc comment short description must end with a full
     |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...erApiLocationServicePlugin/WeatherApiLocationServiceGeonames.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 63 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 63 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...e_weather_api/src/Annotation/WeatherApiLocationServicePlugin.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 38 | ERROR | Class property $base_url should use lowerCamel naming
    |       | without underscores
----------------------------------------------------------------------

Time: 829ms; Memory: 10Mb
alexander.levitsky’s picture

alexander.levitsky commented
Status: Needs work » Needs review

Hi @shaktik

Thank you for your review, the listed issues were fixed.

harishh’s picture

harishh commented

Hi alexander.levitsky,

Looks good. phpcs errors are fixed. Please apply the patch.
https://www.drupal.org/project/flexible_weather_api/issues/3161476

alexander.levitsky’s picture

alexander.levitsky commented

Hi @harishh

Thanks for your patch, it was applied.

alexander.levitsky’s picture

alexander.levitsky commented
Priority: Normal » Major
alexander.levitsky’s picture

alexander.levitsky commented
Priority: Major » Critical
mrweiner’s picture

mrweiner commented
Status: Needs review » Needs work

Checklist

  • Individual user account: yes
  • No duplication: Seems to be unique. There are a variety of weather modules, but as far as I can tell they provide integration with a particular API as opposed to a more generic, pluggable solution.
  • Master Branch: Yes
  • Licensing: yes
  • 3rd party assets: yes
  • README: yes, although it's a bit awkward that the contents of the README link to a file in the project's docs directory in the github repo. It might be more inline with Drupal convention for that file to include the contents directly and/or for the module to have its own documentation page(s) on d.org
  • Code long/complex enough for review: yes
  • Secure code: Looks pretty good but could use another set of eyes. Seems like some sanitization might be good in the controllers' getPageTitle() methods, but I'm unsure if it's totally needed. Also may be worth implementing UrlHelper::stripDangerousProtocols() or UrlHelper::filterBadProtocol() in YahooWeather::buildBaseString if it doesn't break the API integration.
  • Coding style & Drupal API usage: Looks good overall, but it doesn't feel right to have the many \Drupal invocations throughout the OOP plugins, entities, etc. I'm surprised phpcs doesn't treat these as standards violations. For the plugins at least, it would be more inline with the philosophy of dep injection to implement ContainerFactoryPluginInterface and use the create() method to provide those services to the plugins (for like, \Drupal::languageManager() in WeatherApiLocationServiceGeonames.php). Here's a reference for how that would be done.

I don't think the README issue is a blocker but something to consider. Marking Needs Work mostly for the wide use of \Drupal calls and to see if you can sanitize YahooWeather::buildBaseString without breaking things.

mrweiner’s picture

mrweiner commented

Actually, looking at things again, I think that the README.md probably does need to be updated. Check out https://www.drupal.org/node/2181737 for the README template.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Priority: Critical » Normal
alexander.levitsky’s picture

alexander.levitsky commented
Status: Needs work » Needs review

Hello, @mrweiner!

Thank you very much for your detailed review. I have applied some recommendations to the dev branch.

1. The readme was updated.
2. The "\Drupal" calls were replaced at all places where it's possible. Only one place is left - it's a custom entity class Weather Service since I can't find a way how we can implement this and also found the documentation https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio...
where there is a note

Note: It's not possible to inject services to entity object. See this issue for more details.

3. Regarding sanitize YahooWeather::buildBaseString - I just used an example from the documentation for this service
https://developer.yahoo.com/weather/documentation.html#oauth-php

I'm not sure that I should adjust something here. Maybe you have more specific recommendations on what exactly I should do with this method?

Best Regards,
Alex.

ysamoylenko’s picture

ysamoylenko commented
Status: Needs review » Reviewed & tested by the community

Thanks for your contribution!

Review:
* Project's code looks good for me.
* Have not found any issues during module installation and configuration.
* Unit tests are written.

I think that the module is ready to be covered by:
Drupal.org security advisory coverage

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

alexander.levitsky’s picture

alexander.levitsky commented

Hi @kiamlaluno!

Thank you for updating my account and for the useful links! I will strive to continue to contribute to the community.

Best regards,
Alex.

Status: Fixed » Closed (fixed)

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