This module provides a basic integration with the PriceSpider service. Currently this module only provides support for the page-embedded generic Where-to-Buy (WTB) experience.

I did not realize there was another module available before taking maintainership of this one. I just discovered this and have opened an issue to start a dialog about collaborating, see https://www.drupal.org/project/price_spider/issues/3104504.

This module differs from https://git.drupalcode.org/project/price_spider b/c it

  1. Works well with multilingual sites via active language where as price_spider works with a single configured language
  2. Has clear configuration steps in README vs price_spider

Project link

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

Git instructions

git clone --branch 8.x-1.x git@git.drupal.org:project/pricespider.git

PAReview checklist

https://pareview.sh/pareview/https-git.drupal.org-project-pricespider

Comments

jasonawant created an issue. See original summary.

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
CreditAttribution: rksyravi as a volunteer commented

You'll need to resolve all the errors and warnings of the file CHANGELOG.md, pricespider.module, pricespider.info.yml files.
CHANGELOG.md: reduce the characters per line exceeding 80, if exceeded start with the new line to continue.

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
CreditAttribution: rksyravi as a volunteer commented

Also ensure not to use deprecated code in your modules Eg: drupal_set_message() you have used in .module files.

Ankush_03’s picture

Ankush_03
Location Gurgaon, India 🇮🇳
CreditAttribution: Ankush_03 commented
Status: Needs review » Needs work
jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
CreditAttribution: jasonawant at Nerdery for Nestle Purina PetCare - United States commented
Status: Needs work » Needs review

Thanks for the review.

I've removed drupal_set_message usages, see #3079967: deprecated drupal_set_message() method used.

Regarding the other items identified in https://pareview.sh/pareview/https-git.drupal.org-project-pricespider, see below.

These items are found in markdown file, there is ongoing discussion about markdown coding standards, see #2952616: Adopt CommonMark spec for Markdown files. I think the offending lines include links, which shouldn't be flagged, see #2860121: Warning on line length with markdown. If I count characters of the rendered markdown, some do exceed 80 characters. However, the reported line count by pareview will still flag these given it is reporting all characters, e.g. 202 characters...not the 81 rendered markdown characters.

Any recommendations about this?

FILE: ...000/site1101/web/vendor/drupal/pareviewsh/pareview_temp/CHANGELOG.md
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 9 WARNINGS AFFECTING 9 LINES
--------------------------------------------------------------------------
 16 | WARNING | Line exceeds 80 characters; contains 202 characters
 17 | WARNING | Line exceeds 80 characters; contains 203 characters
 18 | WARNING | Line exceeds 80 characters; contains 171 characters
 19 | WARNING | Line exceeds 80 characters; contains 215 characters
 20 | WARNING | Line exceeds 80 characters; contains 219 characters
 21 | WARNING | Line exceeds 80 characters; contains 244 characters
 22 | WARNING | Line exceeds 80 characters; contains 269 characters
 30 | WARNING | Line exceeds 80 characters; contains 230 characters
 33 | WARNING | Line exceeds 80 characters; contains 208 characters
--------------------------------------------------------------------------

Time: 1.92 secs; Memory: 6Mb

I think the following are false positives

FILE: ...te1101/web/vendor/drupal/pareviewsh/pareview_temp/pricespider.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
 255 | WARNING | Variable $sku_field is undefined.
 258 | WARNING | Variable $sku_field is undefined.
--------------------------------------------------------------------------

Time: 1.08 secs; Memory: 6Mb

In the 2nd comment above, there's mention of an issue with the pricespider.info.yml; what's the issue there?

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
CreditAttribution: rksyravi as a volunteer commented

As you can see form the

pareview.sh

in pricespider.module file, you are using function mentioned below:

function pricespider_field_delete($entity_type, $entity, array $field, $instance, $langcode, &$items) {
if ($sku_field == \Drupal::service('pricespider')
    ->getSkuField($entity_type, $bundle)) {
}

you have used $sku_field variable to compare with, but this variable is not defined, you'll need to defined.Eg:

function pricespider_field_delete($entity_type, $entity, array $field, $instance, $langcode, &$items) {
$sku_field = '';
if ($sku_field == \Drupal::service('pricespider')
    ->getSkuField($entity_type, $bundle)) {
}

you can define as array or string or int.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented 
  if ($sku_field == \Drupal::service('pricespider')
    ->getSkuField($entity_type, $bundle)) {
    // Field being deleted, is being used as the sku field?
    if ($sku_field == $field['field_name']) {
      // Remove the field.
      \Drupal::service('pricespider')->setSkuField($entity_type, $bundle);
      \Drupal::messenger()->addWarning(t('No field selected as Sku Field for @type @bundle', [
        '@type' => $entity_type,
        '@bundle' => $bundle,
      ]));
    }
  }

Actually, that code is comparing a variable that has not been initialized with the output of a method (in the first line). Maybe that comparison should be an assignment.

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
CreditAttribution: rksyravi as a volunteer commented

Yeah totally agree with @kiamlaluno, that variable should be an assignment, if not then it's better to define before using further.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
CreditAttribution: vuil at FFW commented
Issue summary: View changes

Thank you for the contribution!

I updated the project's summary. And I set the issue status to Needs work.

Please also fix the following issues:
1. The $sku_field variable is undefined in the 'pricespider.module' file.
2. Please update your 'CHANGELOG.md' file to have no more than 80 characters per line.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
CreditAttribution: vuil at FFW commented
Status: Needs review » Needs work
jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
CreditAttribution: jasonawant at Nerdery for Nestle Purina PetCare - United States commented
Status: Needs work » Needs review

Indeed! I incorrectly saw these as false positives. I've updated as a variable assignment like other usages of this pattern.

See https://pareview.sh/pareview/https-git.drupal.org-project-pricespider

Hi @vuil! Did you comment #5, any recommendations about how to move that forward?

These items are found in markdown file, there is ongoing discussion about markdown coding standards, see #2952616: [policy, no patch] Markdown coding standards (adopt CommonMark spec). I think the offending lines include links, which shouldn't be flagged, see #2860121: Warning on line length with markdown. If I count characters of the rendered markdown, some do exceed 80 characters. However, the reported line count by pareview will still flag these given it is reporting all characters, e.g. 202 characters...not the 81 rendered markdown characters.

Any recommendations about this?

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
CreditAttribution: vuil at FFW commented
Status: Needs review » Needs work

Oh, thank you. Good work!

I found an issue through the manual code review:
- Please, \Drupal calls should be avoided in classes, use dependency injection instead (in src/Routing/PriceSpiderRoutes.php).

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
CreditAttribution: jasonawant at Nerdery for Nestle Purina PetCare - United States commented
Status: Needs work » Needs review

Alright, this is fixed. See https://git.drupalcode.org/project/pricespider/commit/3e8ac9f43733f5c760...

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
CreditAttribution: jasonawant at Nerdery for Nestle Purina PetCare - United States commented

alright, changelog updated! https://www.drupal.org/project/pricespider/issues/3104738

Ankush_03’s picture

Ankush_03
Location Gurgaon, India 🇮🇳
CreditAttribution: Ankush_03 commented
Status: Needs review » Reviewed & tested by the community

Code Looks Good to me Going to RTBC !

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
CreditAttribution: jasonawant at Nerdery for Nestle Purina PetCare - United States commented

Great, thank you everyone for reviewing and all the feedback! What's the next step?

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Assigned: Unassigned » apaderno
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.

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
CreditAttribution: jasonawant at Nerdery for Nestle Purina PetCare - United States commented

Awesome! Thank you everyone for reviewing!

Ankush_03’s picture

Ankush_03
Location Gurgaon, India 🇮🇳
CreditAttribution: Ankush_03 at Srijan | A Material+ Company for Srijan | A Material+ Company commented

Thank you @jasonawant for contribution.

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
CreditAttribution: rksyravi as a volunteer and at Srijan | A Material+ Company for Srijan | A Material+ Company commented

Hi @jasonawant,

You're welcome.

Status: Fixed » Closed (fixed)

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