Comments

pcambra created an issue. See original summary.

govind.maloo’s picture

Status: Active » Needs review
StatusFileSize
new4.27 KB
pcambra’s picture

Status: Needs review » Needs work

Looks generally good, just one thing:

+++ b/src/Form/StockInventoryControlForm.php
@@ -5,10 +5,64 @@ namespace Drupal\commerce_simple_stock\Form;
+   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
+   *   The Query service.
+   * @param \Drupal\Core\Entity\Query\QueryFactory $entityQuery
+   *   The Entity type manager service.
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
+   *   The messenger service.
+   */

I think the labels are mismatched here

kkalaskar’s picture

@govindmaloo

Changes implemented suggested by @pcambra.
Also one more thing, above drupal 8.5 community added MessengerTrait::$messenger in FormBase. It provides a trait for the messenger service. No more need to inject service manually.

FormBase -> https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Form%21Fo...

MessengerTrait -> https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Messenger...

kkalaskar’s picture

Status: Needs work » Needs review
pcambra’s picture

Status: Needs review » Needs work

Thanks @kkalaskar, feel free to implement the trait if you want, we can just add a dependency > 8.5 in the info file.

+++ b/src/Form/StockInventoryControlForm.php
@@ -154,10 +208,10 @@ class StockInventoryControlForm extends FormBase {
+          $this->messenger->addStatus($this->t($productVariation->getTitle() . ': ' . $productVariation->field_stock->value));

I don't think the t() call here is needed, it would translate twice

maliknaik’s picture

StatusFileSize
new1.24 KB

FormBase class has messenger trait added.

maliknaik’s picture

StatusFileSize
new1.23 KB

Patch with t() call removed as mentioned in comment #6

maliknaik’s picture

Status: Needs work » Needs review
kellyimagined’s picture

Patch from #8 works well

kellyimagined’s picture

Status: Needs review » Reviewed & tested by the community
kkalaskar’s picture

Hello everyone,
Added minimum version in info, composer.json file of drupal core.

kkalaskar’s picture

Status: Reviewed & tested by the community » Needs review
pcambra’s picture

Status: Needs review » Needs work

The current patch doesn't apply to the latest dev.

facine’s picture

Status: Needs work » Needs review
StatusFileSize
new1.78 KB

Hi @pcambra, attach a new patch which applies to the latest dev.

  • pcambra committed 0564556 on 8.x-1.x authored by kkalaskar
    Issue #3044858 by kkalaskar, maliknaik, govind.maloo, facine, pcambra:...
pcambra’s picture

Status: Needs review » Fixed

Thanks all!

Status: Fixed » Closed (fixed)

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