Coding standards

I'll assign this to me, as I'm currently reviewing and tidying up a little bit. A patch + comments will follow :)

Woooo, reviewing all the code took quite more time than a thought (plus a bottle of wine :D)

Attached is a patch, that mostly...

• does formatting / code style adjustments like removing empty lines or replacing "array()" with "[]"...
• ...or removing unused use statements or variables
• fixes typos in comments, variable names, etc
• adds missing doc comments (function parameters and return values,..)
• adds stronger typing to function parameters (declare types like array or specific interfaces)
• sometimes does some tiny refactorings... e.g. inject a service within another service rather than using \Drupal::service() call

(some kind of) disclaimer:

What I did, was some very low-level work. I may have missed intentionally or unintentionally some other potential improvements. I'm also not sure, if that meets 100% Drupal coding standards, but it's at least a big leap towards to that.
I haven't installed and used commerce_stock at all. All changes and adjustments I propose in the patch are untested, but I did it cautiously and with good tool support. But reviewing and testing the changes before committing would be a good idea anyway.

Some general impressions gained during review

One impression, that I've got on several places is that the module hasn't fully arrived in D8/Symfony2 yet. This is evident in many function definitons, where several IDs and arrays are passed around, or especially in the storage submodule: until now, I haven't really seen hook_schema implementations in D8, most likely as it's easy as it can be to define custom entity types in D8 -> if you define an custom entity type e.g. for cs_inventory_transaction, you wouldn't have to write much more lines of code in D8. instead of the database-centric hook_schema implementation, you would define your own entity class, add the necessary annoations, base field definitions and additionally all the getter/setter and additional methods you want to have -> with actually not really much more work you'll get a fully typed entitiy class, that you can enrich with any custom functions you like. Additionally, all the parts, where you use the deprecated db_insert() calls, would be even easier to replace - you could use YOUR_ENTITY_CLASS::create([values])->save(); instead and would be fully D8.2 compatible.

Another point, that I've already mentioned before, is that the whole structure of all the classes, services, interfaces, etc defined here, is quite confusing. Of course, the whole topic and the targeted goals of this module cover a very complex scenario, so it's clear, that the structure can get quite complicated of course. But especially in this constellation, one should try to not add too much unnecessary complexity.

This was even hardened by the fact, that the current state of code comments is quite weak. In my patch, I tried to add at least a basic documentation - at least I had to find out the data types of the involved variables, etc. And then I really sometimes got lost somewhere in between all the different classes, that are partly quite tightly knit to each other. Some examples:

On the one hand, I saw some strange circular dependencies: e.g. the StockManager initializes a property holding a StockManagerConfig object in its constructor, which itself takes the StockManager as reference. I don't know, if Symfony2 would find a way to handle this, if both would be services and dependency injection would be used - but e.g. Spring Framework wouldn't compile due to a CircularDependencyException.

And finally one thing, that confused me really very much, is a combination of naming and structure. On the one hand, there's one big directory containing all different interface and class implmentations (see #2726581: better organisation of classes and interfaces), and some classes are implementing several of these interfaces (which is basically ok). On the other hand there some unclear namings, like "AlwaysInStock" and "AlwaysInStockService", which seemed to me to be directly connected to each other, but aren't. I believe, in this case, not the class names are bad, but the fact that they are in the same namespace, listed directly one after another, together with lots of XxxInterface and Xxx class defintions within the same directory/namespace.

And last but not least a very important hint, that I don't want to hide: after having it a very long time on my personal radar without ever trying, we recently decided to try and soon after decide to switch from Eclipse/Aptana to PhpStorm as our preferred IDE. We really fell in love with that tool!!! I believe, that so far I only used a small portion of the possibilites the software offers, but the gain of time, comfort and especially code quality is incredible! I tell you because many of the harmless code style adjustments, as well as problems you wouldn't see without a sophisticated IDE, wouldn't be possible without the usage PhpStorm at all -> I'll open up a separate issue right after this commit about a problem I've found only because of the great validation of that IDE...

You're welcome. I also configured Code Sniffer in my PHP Storm yesterday and was looking for a "victim" to test, if it's working. I remembered our discussion and thought checking this module would be a nice idea :)

regarding the architecture: these are very good arguments :) I must confess that I don't have very much low-level knowledge about what is going on in the Entity API behind the scenes. As there are pre and post save hooks on inserts and updates, the entity approach of course can't be better in performance theoretically. On the other hand, if you design your system so, that none of these hooks are really implemented, there shouldn't be too much difference in performance. And I also don't know, if there's more risk of deadlocks!?? But I'm definitely not the right person to estimate this. Maybe you could ask Wim Leers about that, as he is a real performance ninja, or fago, as he is the founder of Entity API, or bojanz,...

One thing I know about D8 entities - based on my own experience - that you can design them to only reside in a single table. If your entities are non translatable, not fieldable, not revisionable and do not have one to many relationships in their base field definitions, you only will have one table in the database, no extra "data" table like the ones we know from nodes, terms,...

If you don't want entities to be updateable, you can prevent this easily - you could even prevent saving in a pre-save hook :D

In the end, for me as potential developer using the module, it shouldn't make too much difference whether or not you use low-level schema definitions and SQL queries or not, as long as the module's API is powerful enough to cover all the needs and works as expected :-)

Anyhow, I'm looking forward to this module. I was concentrating yesterday much on commenting, removing unneeded blank lines, imports, etc, so that I didn't look too much into the actual semantics of all the bits and bytes I saw, but after all I saw there and read from your posts and comments, I'm pretty sure that it will be a giant evolution step in comparison to the much simpler D7 version...

haha, I believe you. The module is quite big. If code sniffer always complains about the same few superficial rule violations, the report will grow quickly

Great work!

Since the latest commits, we've accrued some coding standards issues, if we are to use Drupal coding standards officially. See https://travis-ci.org/steveoliver/commerce_stock/jobs/175921201.

Can we re-open this issue and resume work?

Ran a check today using phpcs and found over 3k recommended code standards cleanup. I think I'll start here if that's ok.

First pass. Fixes all but the dependency injection code formatting standards. Those will require some more extensive changes to the construct and create methods of the objects.

Second pass. This one fixes a number of places where container injection is recommended to be used (by our coding standards)

After reviewing how that looked once it got up here I noticed I'm rewriting the docblock on a few functions and some curious things came along for the ride. Here's a new patch and let's discuss:

1. +++ b/modules/field/src/Plugin/Field/FieldFormatter/SimpleStockLevelFormatter.php
   @@ -18,7 +22,30 @@ use Drupal\Core\Field\FieldItemListInterface;
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, StockServiceManager $stock_manager) {
   +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
   +    $this->stockServiceManager = $stock_manager;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   +    return new static($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'],
   +      $container->get('commerce_stock.service_manager'));
   +  }
   

   This is the bulk of what I'm trying to do for the SimpleStockLevelFormatter and the SimpleStockLevelWidget. Inject the services that have been brought in by \Drupal

2. +++ b/src/Form/StockConfigForm.php
   @@ -158,6 +181,8 @@ class StockConfigForm extends ConfigFormBase {
   +   * @throws \Drupal\Component\Plugin\Exception\PluginException
   

   My IDE tells me about Exceptions that we should include in the docblocks all the time. Well, usually for cases where we haven't handled an Exception that could come up.

3. +++ b/src/Form/StockConfigForm.php
   @@ -193,4 +218,13 @@ class StockConfigForm extends ConfigFormBase {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getEditableConfigNames() {
   +    return [
   +      'commerce_stock.service_manager',
   +    ];
   +  }
   +
   

   I wasn't expecting StockConfigForm to be missing methods that the ConfigFormBase requires extended objects to override.

thanks @cosmicdreams. Some nitpicks from me

1. +++ b/modules/field/src/Plugin/Field/FieldFormatter/SimpleStockLevelFormatter.php
   @@ -18,7 +22,30 @@ use Drupal\Core\Field\FieldItemListInterface;
   +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, StockServiceManager $stock_manager) {
   +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
   +    $this->stockServiceManager = $stock_manager;
   
   +++ b/modules/field/src/Plugin/Field/FieldWidget/SimpleStockLevelWidget.php
   @@ -20,7 +24,29 @@ use Drupal\Core\Link;
   +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, StockServiceManager $stock_manager) {
   +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $third_party_settings);
   +    $this->stockServiceManager = $stock_manager;
   
   +++ b/modules/ui/src/Form/StockTransactions2.php
   @@ -26,10 +30,24 @@ class StockTransactions2 extends FormBase {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, StockServiceManager $stock_manager, Request $request) {
   +    $this->productVariationStorage = $entity_type_manager->getStorage('commerce_product_variation');
   +    $this->stockServiceManager = $stock_manager;
   

   Should be $stock_service_manager

2. +++ b/modules/ui/src/Form/StockTransactions2.php
   @@ -26,10 +30,24 @@ class StockTransactions2 extends FormBase {
   +    $this->request = $request->getCurrentRequest();
   

   I don't see the declaration of the $request class property?

+++ b/modules/field/src/Plugin/Field/FieldFormatter/SimpleStockLevelFormatter.php
@@ -18,7 +22,30 @@ use Drupal\Core\Field\FieldItemListInterface;
+  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, StockServiceManager $stock_manager) {
+    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
+    $this->stockServiceManager = $stock_manager;
+  }
...
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
+    return new static($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'],
+      $container->get('commerce_stock.service_manager'));
+  }

+++ b/src/Form/StockConfigForm.php
@@ -62,6 +65,26 @@ class StockConfigForm extends ConfigFormBase {
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('config.factory'),
+      $container->get('entity_type.manager'),
+      $container->get('entity_type.bundle.info'),
+      $container->get('commerce_stock.service_manager'),
+      $container->get('plugin.manager.stock_events')
+    );

As far as I know coding standard has no rule for wrapping function declarations and calls. In times of DI the arguments list tends to be undreadable awful long.

I think we should always wrap them if long. But thats Guys decision, i guess.

See #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines for more background on this.

At least we should be consistent. Wrap or not to Wrap - @guy_schneerson - that's your turn

Which one looks better :D

+++ b/modules/field/src/Plugin/Field/FieldFormatter/SimpleStockLevelFormatter.php
@@ -18,7 +22,30 @@ use Drupal\Core\Field\FieldItemListInterface;
 
public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, StockServiceManager $stock_manager) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
    $this->stockServiceManager = $stock_manager;
}

/**
   * {@inheritdoc}
   */
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'],
      $container->get('commerce_stock.service_manager'));
}

public function __construct(
  $plugin_id, 
  $plugin_definition, 
  FieldDefinitionInterface $field_definition, 
  array $settings, 
  $label, 
  $view_mode,
  array $third_party_settings, 
  StockServiceManager $stock_manager
) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
    $this->stockServiceManager = $stock_manager;
}

public static function create( 
   ContainerInterface $container, 
   array $configuration, 
   $plugin_id, 
   $plugin_definition
) {
   return new static(
     $plugin_id, 
     $plugin_definition,
     $configuration['field_definition'], 
     $configuration['settings'], 
     $configuration['label'], 
     $configuration['view_mode'], 
     $configuration['third_party_settings'],
     $container->get('commerce_stock.service_manager')
   );
}

I do prefer the second. I don't mind long documents. I may have an opportunity to make these changes tonight.

I've included those recommendations @olafkarsten. I eventually declined the change to vertical-ize the __construct methods but added the changed to the create methods.

Rerolled the patch and created a PR
https://github.com/BBGuy/commerce_stock/pull/45

otherwise it wouldn't apply after the changes introduced in PR 43/44 if they gets commited before the patch. Otherwise works.

Cool, I'll take a look. In the future, would you prefer a PR via github or a patch via D.O?

https://github.com/BBGuy/commerce_stock/pull/54