[D8] Bandsintown

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxbuenos2729269git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

You don't have any bonus review in your track record. Please take the time to manually review at least three project application and a the link to those projects in this application's summary.

Assigning to myself for review that would probably be tonight

Hi buenos,

Firstly try to resolve the issue in automated review
http://pareview.sh/pareview/httpsgitdrupalorgsandboxbuenos2729269git

PAReview: 3rd party code

bit_widget.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

you can make your js file directly external like I did in sharethis module. so the changes would be
bandsintown.libraries.yml from :

bit_widget:
  js:
    libraries/js/bit_widget.js: {}

To

bit_widget:
  js:
    http://widget.bandsintown.com/javascripts/bit_widget.js: {}

OR use the libraries module for dependencies.
Unassigning from myself. will pick it after the changes will be done, will review some other application.

Edit:
Another major one :

core key is missing in .info.yml file.

add core: 8.x in bandsintown.info.yml file.

Automated Review

Review of the 8.x-1.x branch (commit 0f03ff5):

• PHP Fatal error: Can't use method return value in write context in ./src/Plugin/Field/FieldType/BandsintownItem.php on line 46 Errors parsing ./src/Plugin/Field/FieldType/BandsintownItem.php

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements IMHO.

Coding style & Drupal API usage

1. (*) In BandsintownItem.php... in the method called propertyDefinitions(), the $properties variable is not initialized. I believe the code should be changed from
   

     public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
       $bandsintown_settings = bandsintown_bandsintown_settings();
   
       foreach ($bandsintown_settings as $key => $setting) {
         $properties[$key] = DataDefinition::create($setting['type'])
           ->setLabel($setting['desc']);
       }
       return $properties;
     }
   

   to

     public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
       $properties = parent::propertyDefinitions($field_definition);
       $bandsintown_settings = bandsintown_bandsintown_settings();
   
       foreach ($bandsintown_settings as $key => $setting) {
         $properties[$key] = DataDefinition::create($setting['type'])
           ->setLabel($setting['desc']);
       }
       return $properties;
     }
   

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Manual Review :

• bandsintown_bandsintown_settings: Add a new service bandsintown.helper/bandsintown.manager and movie this public function into it instead of maintaining the procedural code. Move this function bandsintown_is_bandsintown too into this service.
• bandsintown.schema.yml : Nice++
• BandsintownBlock.php : build : Use $this->getConfiguration() instead of directly using $this->configuration
• BandsintownBlock.php : blockValidate() : Add a dedicated issue on d.o. and link it in @todo what exactly is pending.
• BandsintownBlock.php : blockSubmit() : how's configuration is saving. did you forget to save the configuration by its ->save call ?
• BandsintownWidget.php : validate : same as above BandsintownBlock.php blockValidate()

Nothing blocker found. Assigning to mpdonadio to give it a final look when he'll get time.

Which version of Drupal 8 are you using. I am using 8.1.0 and the line $properties = parent::propertyDefinitions($field_definition); works just fine for me on that version. This is strange.

Ok That explains it. I will upgrade and see how it goes.

Sorry for the delay; dealing with a personal issue.

Automated Review

Review of the 8.x-1.x branch (commit b464fa9):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Get rid of the TODO.txt and make issues about the things the module needs. That way you can tie commits back to the issue.

BandsintownFormSettings should use injected dependencies (eg, link generator and translation) instead of statics methods and globals. Also in other classes.

I think the variables in the Twig are all OK; I can't exploit them (because quotes and gt/lt are escaped), but I think the
iframe path should really be built up as a Url() object and passed in instead as a best practice.

There is an interesting security issue here. The libraries.yml has an external library with an explicit http path in it. This file supports protocol relative URLs (see https://www.drupal.org/theme-guide/8/assets#external). As-is, this would create a mixed mode problem for a https-only site. However, the certificate on https://widget.bandsintown.com/javascripts/bit_widget.js is bad because it is really coming from a CloundFront via a CNAME, and the CF end isn't set up with the cert for widget.bandsintown.com. So, the https version shouldn't really be used. If I don't hear back from @klausi on this tomorrow, I will approve this app.

OK, since the JS in question is really for a read-only purpose, I don't think we have a true security problem here, though HTTPS sites using this widget will have mixed mode problems. You should open an issue with bandsintown about this; they should really fix this on their end, and then you can use the protocol-relative URL in your YAML. So, since I don't think we have any blockers...

Thanks for your contribution, buenos!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.