# Summary

Tested and works with Drupal 8.0.0-rc2 Still waiting on 1.0 release mentioned in https://www.drupal.org/node/2229539#comment-10528834

# Project URL


# Where is the code?


# Estimated completion date


# Dependencies


# Who's doing the port?


# What help do they need?


# D8 roadmap


# Background and reference information



kreynen created an issue. See original summary.

skein’s picture

Status: Needs review » Needs work

Hi there.

I took at your 8.x module

Some things I found:

1. Drupal standards:
Ran the branch in http://pareview.sh/. It showed quite a few issues. The list s too long to post here.

2. If jQuery.Fitvids plugin is a 3rd party library, shouldn't it live in the libraries folder and be loaded with the libraries API and not be packaged up with the module itself?

3. A lot of commented out code from the d7 version. Shouldn't it be removed?

4. hook_permissions is still defined and should be removed.

5. For consistency with other modules shouldn't the css file be placed into a css folder?

6. No schema file for the module settings

7. console.log should be removed from the init js file

8. Not sure if the make example still needs to be there

9. In fitvids_page_attachments you call settings 3 times like this:

$selectors = \Drupal::config('fitvids.settings')->get('selectors');

Probably would be better to save the configuration into a variable and use it to get the values like you do in the form saving part of the module

10. In the if statement:

if (strlen(trim($custom_vendors))) {

strlen is an unneed call. if trim will return an empty string it will evaluate as FALSE

11. Not 100% sure about this one:

implode(',', explode(PHP_EOL, $selectors));

Seems like this would be a better fit since it will only call one function:

str_replace(PHP_EOL, ',', $selectors));

12. Constants defined in the beginning of the module file are not used

13. In FitvidsAdminSettingsForm.php remove unused use options

14. You are using t() function directly in the ConfigForm. Should be $this->t()

15. Probably the install file is not needed any more

16. Got this a few times:
Recoverable fatal error: Argument 2 passed to fitvids_help() must be an instance of RouteMatchInterface, instance of Drupal\Core\Routing\CurrentRouteMatch given in fitvids_help() (line 22 of modules/fitvids/fitvids.module).

DerekAhmedzai’s picture

Status: Needs work » Needs review

There is a new release for testing - 8.x-1.0-rc1

mgifford’s picture