Problem/Motivation

Follow-up to : #2889425: Porting Uc wishlist module to Drupal 8 | Phase 1 & #2880447: Porting Uc wishlist module to Drupal 8 | Phase 2

Proposed resolution

  1. uc_wishlist.info.yml: configure: admin/store/settings/wishlist Configure link should point to a route name.
  2. uc_wishlist.install: Remove uc_wishlist_update_7001
    /**
 * Replaced the anonymous configuration settings with 'create wish lists'
 * permissions.
 */
function uc_wishlist_update_7001() {
  $allow_anonymous = \Drupal::config()->get('uc_wishlist_allow_anonymous', FALSE);
  if ($allow_anonymous) {
    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, ['create wish lists' => TRUE]);
  }
  variable_del('uc_wishlist_allow_anonymous');
}
  3. uc_wishlist.links.task.yml:
    uc_wishlist.tab_1:
  route_name: uc_wishlist.wishlist.email_form
  title: Email
  base_route: uc_wishlist.wishlist.email_form
  weight:

    Remove the weight key and rename the menu task key to uc_wishlist.email

  4. uc_wishlist.module uc_wishlist_form_alter: $database = new DBQuery(\Drupal::database()); Convert the DBQuery class from a helper class to a service and inject the database connection service into it and call the service using \Drupal::service("servicename");
  5. uc_wishlist.module uc_wishlist_form_alter:uc_order_load($_SESSION['cart_order']); This is not a function in ubercart 8.x use Order::load instead
  6. uc_wishlist.module uc_wishlist_remove_from_wishlist_submit: drupal_set_message(t('<b>@product_title</b> was removed from <a href="@url">your wish list</a>.',['@product_title'=>$productTitle,'@url'=>\Drupal\Core\Url::fromRoute('uc_wishlist.wishlist')->toString()])); Remove the html from the translate method and add a use statement of the Url class at the top and use the Url class instead of whole namespace.
  7. uc_wishlist.module uc_wishlist_create_wishlist:
        drupal_set_message(t('You must be logged in to create a wish list. Please <a href="@login_url">login</a> or <a href="@register_url">register</a>.',
        array(
         '@login_url' => Drupal\Core\Url::fromRoute('user.login'),
         '@register_url' => Drupal\Core\Url::fromRoute('user.register')
        )));

    Add a use statement of the Url class at the top and use the Url class instead of whole namespace.

  8. uc_wishlist.module uc_wishlist_create_wishlist: $config = \Drupal::config('uc_wishlist'); Which configuration is it returning? It should be \Drupal::config('uc_wishlist.settings')
  9. uc_wishlist.module uc_wishlist_add_item:
        if (!$wid) {
      drupal_set_message($this->t('Could not create wish list. Adding item failed.'), 'error');
      return FALSE;
    }

    What's $this here?

  10. uc_wishlist.module uc_product_update_wishlist_item: uc_wishlist_remove_item($wpid); Where's this uc_wishlist_remove_item function defined.
  11. uc_wishlist.module uc_wishlist_views_api: hook_views_api has been removed in drupal 8 see https://www.drupal.org/node/1875596
  12. uc_wishlist.permissions.yml:
    administer wish lists:
  title: 'administer wish lists'
  description: 'Allows editing or deletion of any wish list.'
create wish lists:
  title: 'create wish lists'
  description: 'Allow creation and editing of own wish list.'

    Update the description of the administer wish lists that reflect what this permissions does.

  13. uc_wishlist.permission.yml: Why this file is required?
  14. uc_wishlist.routing.yml:
    uc_wishlist.settings:
  path: '/admin/store/config/wishlist'
  defaults:
    _form: '\Drupal\uc_wishlist\Form\UcWishlistConfigForm'
    _title: 'Wish list settings'
  requirements:
    _permission: 'Administer wish list settings.'
  options:
    _admin_route: TRUE

    Wrong permission name. It should be the key of the permission

  15. uc_wishlist.rules.inc:
    function uc_wishlist_condition_product_wishlist($order, $settings) {
  return uc_wishlist_order_wishlist($order);
}

    Where uc_wishlist_order_wishlist is defined?

  16. src/UcWishlistListBuilder.php: Where this class has been used?
  17. src/Entity/UcWishlist.php: Where this class has been used?
  18. src/Database/DBQuery.php: Convert this class to a service. See how to create services https://www.drupal.org/docs/8/api/services-and-dependency-injection/serv...
  19. src/Form/UCWishlistAdminDeleteForm.php: Add docblock to these properties
      protected $user;
  protected $wishlistId;
  20. src/Form/UCWishlistAdminDeleteForm.php:
      public function buildForm(array $form, FormStateInterface $form_state, $wishlist = NULL) {
    $this->node = $node;
    $this->featureId = $fid;
    $this->feature = uc_product_feature_load($pfid);
    $wishlist = uc_wishlist_load($wishlist);
    $form['wishlist'] = [
      '#type' => 'value',
      '#value' => $wishlist,
    ];
    return parent::buildForm($form, $form_state);
  }

    From where the $node and $fid are coming?

  21. src/Form/UCWishlistAdminDeleteForm.php:
      public function submitForm(array &$form, FormStateInterface $form_state) {
    $values = $form_state->getValues();
    $database = new DBQuery(\Drupal::database());
    $database->deleteWishlist($values['wishlist']->wid);
    drupal_set_message(t('@title has been deleted.', ['@title' => str_replace('.','',$values['wishlist']->title)]));
    $form_state->setRedirect('uc_wishlist.admin_wishlist');
  }

    Use $this->t instead of t function directly

  22. src/Form/WishlistEmailForm.php:
        $emails = explode(',', $form_state->getValue['recipients']);

    getValue does not have any property at formstate class.

  23. src/Form/WishlistEmailForm.php: $form_state->setError('emails', $this->t('%email is not a valid email address', ['%email' => $email])); does emails element exist on the form?
  24. src/Form/WishlistSearchForm.php: Remove unused use statements from the class.
  25. src/Form/WishlistSearchForm.php: Remove unused commented code.
      //protected $database;

  /**
   *public function __construct() {
      $this->db = new Database\DBQuery(\Drupal::database());
    }
   *
   */
  26. src/Form/WishlistSearchForm.php: buildForm:
        $form['search']['keywords'] = [
      '#type' => 'textfield',
      '#title' => t('Search keywords'),
      '#description' => t('Enter the keywords to use to search wish list titles and addresses.'),
      '#default_value' => $keywords,
    ];

    From where this $keywords variable is coming?

  27. src/Form/WishlistSearchForm.php: buildForm:
        foreach ($result as $wishlist) {
      $links[] = [
        'title' => Xss::filter($wishlist->title, []),
        'href' => 'wishlist/' . $wishlist->wid,
      ];
    }

    From where did $result variable is coming?

  28. src/Form/WishlistSearchForm.php: submitForm: Formstate class doesn't have getValue property. It's getValue method. Referring code: if (empty($form_state->getValue['keywords'])) {
  29. src/Form/WishlistViewForm.php: Remove the commented code and unused code from the class.
  30. src/Form/WishlistViewForm.php: buildForm: $form['#attached']['library'][] = 'uc_wishlist/ideal_image_slider'; Where's this library is defined?
  31. src/Form/WishlistViewForm.php: Undefined class Node in the file. Add a use statement for the Node class.
  32. src/Form/WishlistViewForm.php: submitForm: $this->database->remove_item($wpid); Where remove_item is defined?
  33. src/Form/WishlistViewForm.php: submitForm:$database = new \Drupal\uc_wishlist\Database\DBQuery(\Drupal::database()); Add a Use statement of the DBQuery class and use the DBQuery instead of whole namespace

Remaining tasks

Provide the patch to fix them.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#50 interdiff-2904915-49-50.txt50.12 KBnaveenvalecha
#50 2904915-50.patch79.3 KBnaveenvalecha
#49 2904915-final-fix-5.patch46.25 KBchiranjeeb2410
#47 Screen Shot 2017-09-05 at 6.17.09 PM.png108.56 KBnaveenvalecha
#46 2904915-final-fix-4.patch46.25 KBchiranjeeb2410
#43 2904915-final-fix-3.patch45.87 KBchiranjeeb2410
#42 2904915-final-fix-3.patch45.87 KBchiranjeeb2410
#39 2904915-final-fix-2.patch40.97 KBchiranjeeb2410
#32 2904915-final-fix.patch37.3 KBchiranjeeb2410
#30 2904915-30.patch3.37 KBchiranjeeb2410
#26 2904915-26.patch18.16 KBchiranjeeb2410
#21 2904915-21.patch17.6 KBchiranjeeb2410
#17 2904915-17.patch18.32 KBchiranjeeb2410
#13 2904915-13.patch14.44 KBchiranjeeb2410
#8 2904915-8.patch12.06 KBchiranjeeb2410
#6 2904915-6.patch12.31 KBchiranjeeb2410
#4 2904915-4.patch11.75 KBchiranjeeb2410
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naveenvalecha created an issue. See original summary.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Issue summary: View changes

Here's the round 1 of review of the 8.x-1.x branch

  1. uc_wishlist.info.yml: configure: admin/store/settings/wishlist Configure link should point to a route name.
  2. uc_wishlist.install: Remove uc_wishlist_update_7001
    /**
 * Replaced the anonymous configuration settings with 'create wish lists'
 * permissions.
 */
function uc_wishlist_update_7001() {
  $allow_anonymous = \Drupal::config()->get('uc_wishlist_allow_anonymous', FALSE);
  if ($allow_anonymous) {
    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, ['create wish lists' => TRUE]);
  }
  variable_del('uc_wishlist_allow_anonymous');
}
  3. uc_wishlist.links.task.yml:
    uc_wishlist.tab_1:
  route_name: uc_wishlist.wishlist.email_form
  title: Email
  base_route: uc_wishlist.wishlist.email_form
  weight:

    Remove the weight key and rename the menu task key to uc_wishlist.email

  4. uc_wishlist.module uc_wishlist_form_alter: $database = new DBQuery(\Drupal::database()); Convert the DBQuery class from a helper class to a service and inject the database connection service into it and call the service using \Drupal::service("servicename");
  5. uc_wishlist.module uc_wishlist_form_alter:uc_order_load($_SESSION['cart_order']); This is not a function in ubercart 8.x use Order::load instead
  6. uc_wishlist.module uc_wishlist_remove_from_wishlist_submit: drupal_set_message(t('<b>@product_title</b> was removed from <a href="@url">your wish list</a>.',['@product_title'=>$productTitle,'@url'=>\Drupal\Core\Url::fromRoute('uc_wishlist.wishlist')->toString()])); Remove the html from the translate method and add a use statement of the Url class at the top and use the Url class instead of whole namespace.
  7. uc_wishlist.module uc_wishlist_create_wishlist:
        drupal_set_message(t('You must be logged in to create a wish list. Please <a href="@login_url">login</a> or <a href="@register_url">register</a>.',
        array(
         '@login_url' => Drupal\Core\Url::fromRoute('user.login'),
         '@register_url' => Drupal\Core\Url::fromRoute('user.register')
        )));

    Add a use statement of the Url class at the top and use the Url class instead of whole namespace.

  8. uc_wishlist.module uc_wishlist_create_wishlist: $config = \Drupal::config('uc_wishlist'); Which configuration is it returning? It should be \Drupal::config('uc_wishlist.settings')
  9. uc_wishlist.module uc_wishlist_add_item:
        if (!$wid) {
      drupal_set_message($this->t('Could not create wish list. Adding item failed.'), 'error');
      return FALSE;
    }

    What's $this here?

  10. uc_wishlist.module uc_product_update_wishlist_item: uc_wishlist_remove_item($wpid); Where's this uc_wishlist_remove_item function defined.
  11. uc_wishlist.module uc_wishlist_views_api: hook_views_api has been removed in drupal 8 see https://www.drupal.org/node/1875596
  12. uc_wishlist.permissions.yml:
    administer wish lists:
  title: 'administer wish lists'
  description: 'Allows editing or deletion of any wish list.'
create wish lists:
  title: 'create wish lists'
  description: 'Allow creation and editing of own wish list.'

    Update the description of the administer wish lists that reflect what this permissions does.

  13. uc_wishlist.permission.yml: Why this file is required?
  14. uc_wishlist.routing.yml:
    uc_wishlist.settings:
  path: '/admin/store/config/wishlist'
  defaults:
    _form: '\Drupal\uc_wishlist\Form\UcWishlistConfigForm'
    _title: 'Wish list settings'
  requirements:
    _permission: 'Administer wish list settings.'
  options:
    _admin_route: TRUE

    Wrong permission name. It should be the key of the permission

  15. uc_wishlist.rules.inc:
    function uc_wishlist_condition_product_wishlist($order, $settings) {
  return uc_wishlist_order_wishlist($order);
}

    Where uc_wishlist_order_wishlist is defined?

  16. src/UcWishlistListBuilder.php: Where this class has been used?
  17. src/Entity/UcWishlist.php: Where this class has been used?
  18. src/Database/DBQuery.php: Convert this class to a service. See how to create services https://www.drupal.org/docs/8/api/services-and-dependency-injection/serv...
  19. src/Form/UCWishlistAdminDeleteForm.php: Add docblock to these properties
      protected $user;
  protected $wishlistId;
  20. src/Form/UCWishlistAdminDeleteForm.php:
      public function buildForm(array $form, FormStateInterface $form_state, $wishlist = NULL) {
    $this->node = $node;
    $this->featureId = $fid;
    $this->feature = uc_product_feature_load($pfid);
    $wishlist = uc_wishlist_load($wishlist);
    $form['wishlist'] = [
      '#type' => 'value',
      '#value' => $wishlist,
    ];
    return parent::buildForm($form, $form_state);
  }

    From where the $node and $fid are coming?

  21. src/Form/UCWishlistAdminDeleteForm.php:
      public function submitForm(array &$form, FormStateInterface $form_state) {
    $values = $form_state->getValues();
    $database = new DBQuery(\Drupal::database());
    $database->deleteWishlist($values['wishlist']->wid);
    drupal_set_message(t('@title has been deleted.', ['@title' => str_replace('.','',$values['wishlist']->title)]));
    $form_state->setRedirect('uc_wishlist.admin_wishlist');
  }

    Use $this->t instead of t function directly

  22. src/Form/WishlistEmailForm.php:
        $emails = explode(',', $form_state->getValue['recipients']);

    getValue does not have any property at formstate class.

  23. src/Form/WishlistEmailForm.php: $form_state->setError('emails', $this->t('%email is not a valid email address', ['%email' => $email])); does emails element exist on the form?
  24. src/Form/WishlistSearchForm.php: Remove unused use statements from the class.
  25. src/Form/WishlistSearchForm.php: Remove unused commented code.
      //protected $database;

  /**
   *public function __construct() {
      $this->db = new Database\DBQuery(\Drupal::database());
    }
   *
   */
  26. src/Form/WishlistSearchForm.php: buildForm:
        $form['search']['keywords'] = [
      '#type' => 'textfield',
      '#title' => t('Search keywords'),
      '#description' => t('Enter the keywords to use to search wish list titles and addresses.'),
      '#default_value' => $keywords,
    ];

    From where this $keywords variable is coming?

  27. src/Form/WishlistSearchForm.php: buildForm:
        foreach ($result as $wishlist) {
      $links[] = [
        'title' => Xss::filter($wishlist->title, []),
        'href' => 'wishlist/' . $wishlist->wid,
      ];
    }

    From where did $result variable is coming?

  28. src/Form/WishlistSearchForm.php: submitForm: Formstate class doesn't have getValue property. It's getValue method. Referring code: if (empty($form_state->getValue['keywords'])) {
  29. src/Form/WishlistViewForm.php: Remove the commented code and unused code from the class.
  30. src/Form/WishlistViewForm.php: buildForm: $form['#attached']['library'][] = 'uc_wishlist/ideal_image_slider'; Where's this library is defined?
  31. src/Form/WishlistViewForm.php: Undefined class Node in the file. Add a use statement for the Node class.
  32. src/Form/WishlistViewForm.php: submitForm: $this->database->remove_item($wpid); Where remove_item is defined?
  33. src/Form/WishlistViewForm.php: submitForm:$database = new \Drupal\uc_wishlist\Database\DBQuery(\Drupal::database()); Add a Use statement of the DBQuery class and use the DBQuery instead of whole namespace
naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Priority: Normal » Major

Bumping this to major because the module is throwing errors and not usable.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Active » Needs review
FileSize
2904915-4.patch11.75 KB

@naveenvalecha,

Fresh patch addresses majority of the issues previously listed. Created services for dbquery helper class, however working on injecting it in .module file. Also, issue for viewing wishlist after adding products has been resolved.

Working on the remaining ones.

Please review.

Status: Needs review » Needs work

The last submitted patch, 4: 2904915-4.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-6.patch12.31 KB

Status: Needs review » Needs work

The last submitted patch, 6: 2904915-6.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-8.patch12.06 KB

Status: Needs review » Needs work

The last submitted patch, 8: 2904915-8.patch, failed testing. View results

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

@chiranjeeb2410,

Fresh patch addresses majority of the issues previously listed.

Specify in the comment which points have you addressed in the patch #8 which will help me to understand.

Although the patch is not applying.
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

patch #8 adrresses the points 1, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 19, 20, 21, 24, 25, 27, 29, 31, 32. 33.

Patch is not applying due to some probable errors. Working on the fixing the remaining issues and would upload a
correct patch today asap.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

sounds good

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-13.patch14.44 KB
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

Patch #13 adrresses the points 1, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 18, 19, 20, 21, 24, 25, 27, 29, 31, 32. 33.

Please review.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work 
  1. +++ b/uc_wishlist.info.yml
@@ -3,7 +3,6 @@ type: module
-configure: admin/store/settings/wishlist

    Add the route name in the configure link See user.info.yml for reference https://api.drupal.org/api/drupal/core%21modules%21user%21user.info.yml/... 

  2. +++ b/uc_wishlist.links.tasks.yml
@@ -1,5 +1,9 @@
+  base_route: uc_wishlist.admin.store.customers

    Where is this base route defined? 

  3. +++ b/uc_wishlist.module
@@ -169,7 +169,8 @@ function uc_wishlist_remove_from_wishlist_submit($form, FormState $form_state)
+  drupal_set_message(t('@product_title was removed from your wish list.',

    Add a use statement of Url class at the top of the file and use only Url::fromRoute method instead of whole name space. 

  4. +++ b/uc_wishlist.permissions.yml
@@ -1,12 +1,13 @@
+  title: 'administer wish lists'

    Why this change? 

  5. +++ b/uc_wishlist.permissions.yml
@@ -1,12 +1,13 @@
+  title: 'access wish lists'

    Why change in the permission title? 

  6. +++ b/uc_wishlist.routing.yml
@@ -4,7 +4,7 @@ uc_wishlist.settings:
+    _permission: 'administer wish lists'

    looks good :) 

  7. +++ b/uc_wishlist.routing.yml
@@ -14,7 +14,7 @@ uc_wishlist.admin_wishlist:
+    _permission: 'manage user wish lists'

    Where is this permission defined? 

  8. +++ b/uc_wishlist.routing.yml
@@ -67,10 +67,29 @@ uc_wishlist.wishlist.search:
+    _permission: 'access content'

    use "access wish lists" permission here 

  9. +++ b/uc_wishlist.services.yml
@@ -0,0 +1,3 @@
+  uc_wishlist.database_query:

    rename this service to "uc_wishlist.manager" 

  10. +++ b/uc_wishlist.services.yml
@@ -0,0 +1,3 @@
+    class: Drupal\uc_wishlist\Database\DBQuery

    Rename DBQuery class to UcWishlistManager class and inject the connection object into it. See user.services.yml
    Here's one of the service
    user.data:
    class: Drupal\user\UserData
    arguments: ['@database']

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

On it.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-17.patch18.32 KB

@naveenvalecha,

Addressed all the listed issues in #16.

Please review.

Status: Needs review » Needs work

The last submitted patch, 17: 2904915-17.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review

Uploading correct patch asap

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs review » Needs work
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-21.patch17.6 KB

Status: Needs review » Needs work

The last submitted patch, 21: 2904915-21.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review

@naveenvalecha,

for fix 10 in #15, the DBQuery class name in all the files should be renamed to UcWishlistManager ?

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work

for fix 10 in #15, the DBQuery class name in all the files should be renamed to UcWishlistManager ?

If you're renaming a class then its usage should also be replaced.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

Did that for the previous 2 patches, but there are some other probable errors I guess. Will fix and post a fresh patch soon.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-26.patch18.16 KB

Status: Needs review » Needs work

The last submitted patch, 26: 2904915-26.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

Addressed all the issues but patch doesnt seem to apply again. Could you check the code once ?

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

@chiranjeeb2410
post the workable patch. See how to make the patch and apply the patch https://www.drupal.org/patch/apply

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-30.patch3.37 KB

Status: Needs review » Needs work

The last submitted patch, 30: 2904915-30.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-final-fix.patch37.3 KB
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

Addressed the listed issues for #2 and all the issues for #15 in fresh patch. Kindly clone the uc_wishlist repo
and apply the patch.

Please review.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work 
  1. +++ b/src/Controller/UCWishlistController.php
@@ -16,7 +16,7 @@ class UCWishlistController extends ControllerBase {
+    $this->db = new Database\UcWishlistManager(\Drupal::database());

    Inject the UCWishlistManager service in the controller. See https://www.drupal.org/docs/8/api/services-and-dependency-injection/serv...
    Also, see this example in a UserController https://api.drupal.org/api/drupal/core%21modules%21user%21src%21Controll... 

  2. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add a comment here. "Construct an UcWishlistManager object."
    Also, add the param documentation. https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... 

  3. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+  public function getAllWishlist()   {

    Add the doc block on this function. See coding standards https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... 

  4. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add the appropriate comment here 

  5. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    same as above. 

  6. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    same as above. 

  7. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    same as above. 

  8. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    same as above. 

  9. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    same as above. 

  10. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+

    Add the docblock to this function. 

  11. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add the documentation for this function. 

  12. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add the documentation for this function. 

  13. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add the documentation for this function. 

  14. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add the documentation for this function. 

  15. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+   *

    Add the documentation for this function. 

  16. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,164 @@
+  public function queryExamples() {

    Is it required? if not, remove it. 

  17. +++ b/src/Form/UCWishlistAdminDeleteForm.php
@@ -69,9 +73,9 @@ class UCWishlistAdminDeleteForm extends ConfirmFormBase {
+    $database = new UcWishlistManager(\Drupal::database());

    Inject the uc_wishlist.manager service. See https://www.drupal.org/docs/8/api/services-and-dependency-injection/depe... 

  18. +++ b/src/Form/WishlistViewForm.php
@@ -276,16 +274,13 @@ class WishlistViewForm extends FormBase {
+        $database = new UcWishlistManager(\Drupal::database());

    Inject this service in the form. See how to inject it https://www.drupal.org/docs/8/api/services-and-dependency-injection/depe... 

  19. +++ b/uc_wishlist.links.menu.yml
@@ -14,9 +14,15 @@ uc_wishlist.wishlist:
+  parent: uc_wishlist.wishlist

    Where is this parent defined? 

  20. +++ b/uc_wishlist.module
@@ -75,7 +75,7 @@ function uc_wishlist_form_alter(&$form, FormState $form_state, $form_id) {
+    $database = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to call this service. 

  21. +++ b/uc_wishlist.module
@@ -139,7 +139,7 @@ function uc_wishlist_form_alter(&$form, FormState $form_state, $form_id) {
+          $order = Order::load($_SESSION['cart_order']);

    Where's the use statement of the Order class? 

  22. +++ b/uc_wishlist.module
@@ -164,12 +164,13 @@ function uc_wishlist_form_alter(&$form, FormState $form_state, $form_id) {
+  $database = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to call this service. 

  23. +++ b/uc_wishlist.module
@@ -215,7 +216,7 @@ function uc_wishlist_create_wishlist($title = NULL) {
+  $db = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to instantiate the object of this service class. 

  24. +++ b/uc_wishlist.module
@@ -228,7 +229,7 @@ function uc_wishlist_create_wishlist($title = NULL) {
+  $db = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to instantiate the object of this service class. 

  25. +++ b/uc_wishlist.module
@@ -439,7 +439,7 @@ function uc_wishlist_get_wid($uid = NULL) {
+  $db = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to instantiate the object of this service class. 

  26. +++ b/uc_wishlist.module
@@ -481,7 +481,7 @@ function uc_wishlist_load($wid) {
+  $dbQuery = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to instantiate the object of this service class. 

  27. +++ b/uc_wishlist.module
@@ -500,7 +500,7 @@ function uc_wishlist_get_contents($wid = NULL) {
+  $database = new UcWishlistManager(\Drupal::database());

    use \Drupal::service to instantiate the object of this service class. 

  28. +++ b/uc_wishlist.routing.yml
@@ -65,12 +65,31 @@ uc_wishlist.wishlist.search:
\ No newline at end of file

    Add a newline at EOF

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

On it.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

For statically accessing the service in .module file, 

 $database = new UcWishlistManager(\Drupal::database());

would be changed to 

$database = \Drupal::service('uc_wishlist.manager');

?

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

#36: Yes

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@naveenvalecha,

Okay, finished those parts regarding DI in forms and controllers, working on the documentation part
now. Will upload a patch shortly.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2904915-final-fix-2.patch40.97 KB

@naveenvalecha,

Addressed all the above listed issues in #34.

Please review.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work

Great progress, this is near to RTBC.

  1. +++ b/src/Controller/UCWishlistController.php
@@ -6,17 +6,30 @@ use Drupal\Core\Controller\ControllerBase;
   protected $db;

    rename this property to $wishlistManager because it better reflects the service name. 

  2. +++ b/src/Controller/UCWishlistController.php
@@ -6,17 +6,30 @@ use Drupal\Core\Controller\ControllerBase;
+  public function __construct(UcWishlistManager $db) {

    rename $db to $wishlist_manager 

  3. +++ b/src/Controller/UCWishlistController.php
@@ -6,17 +6,30 @@ use Drupal\Core\Controller\ControllerBase;
+    $this->db = $db;

    $this->wishlistManager = $wishlist_manager; 

  4. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * UcWishlistManager constructor.

    Constructs a UcWishlistManager object. 

  5. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add a description/comment of the function above the return statement. 

  6. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $result = $query->execute();//$query->extend('PagerDefault')->limit(25)->execute();

    Remove the commented line and add a comment in next line with a todo of adding the pager 

  7. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @param $fields

    Add the descriptions of the $fields and $values property. 

  8. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    return $query = $this->connection->insert('uc_wishlists')->fields($fields,$values)->execute();

    What's the use of $query variable? If it's not required, remove it. 

  9. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @param $fields

    Add the descriptions of the $fields and $values property. 

  10. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    return $query = $this->connection->insert('uc_wishlist_products')->fields($fields,$values)->execute();

    What's the use of $query variable? If it's not required, remove it. 

  11. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @param $uid

    Add the descriptions of the function. 

  12. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type of the function. 

  13. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $query = $this->connection->query("SELECT wid FROM {uc_wishlists} WHERE uid = :uid", [':uid' => $uid])->fetchField();
+    return $query;

    What's the use of $query variable? If it's not required, remove it. 

  14. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @param $wid

    Add the descriptions of the function. 

  15. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @param $data

    Add the description of the $data 

  16. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type after @return. Follow the coding standards document 

  17. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $query = $this->connection->query("SELECT * FROM {uc_wishlist_products} WHERE wid = :wid AND nid = :nid AND data = :data", [':wid' => $wid, ':nid' => $nid, ':data' => serialize($data)]);
+    return $query->fetchObject();

    What's the use of $query variable? If it's not required, remove it. 

  18. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description of the function. 

  19. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  20. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $result = $query->orderBy('w.title')->execute; // $query->extend('PagerDefault')->limit(25)->execute();

    Remove the commented line and add a comment in next line with a todo of adding the pager 

  21. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description of the function. 

  22. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type 

  23. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $query = $this->connection->query("SELECT * FROM {uc_wishlists} WHERE wid = :wid", [':wid' => $wid]);
+    return $query;

    What's the use of $query variable? If it's not required, remove it. 

  24. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  25. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @param $rid
+   * @param $created

    Add the description to the $rid and $created variable. 

  26. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  27. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  28. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  29. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  30. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $this->connection->update('uc_wishlist_products')->fields(['qty'=>$qty])->condition('wpid',$wpid,'=')->execute();

    What are you returning here? 

  31. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  32. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  33. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  34. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  35. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+    $query = $this->connection->query("SELECT * FROM {uc_wishlist_products} WHERE nid = :pid AND wid = :wid", [':pid'=>$pid,':wid'=>$wid]);
+    return $query->fetchAll();

    What's the use of $query variable? If it's not required, remove it. 

  36. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  37. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  38. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+  /**

    Add the description to the function. 

  39. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,245 @@
+   * @return

    Add the return type. 

  40. +++ b/src/Form/UCWishlistAdminDeleteForm.php
@@ -4,14 +4,43 @@
+  protected $database;

    Property name should reflect the service name. Rename it to $ucwishlistManager 

  41. +++ b/src/Form/UCWishlistAdminDeleteForm.php
@@ -4,14 +4,43 @@
+  public function __construct(UcWishlistManager $database) {

    UcWishlistManager $ucwishlist_manager 

  42. +++ b/src/Form/UCWishlistAdminDeleteForm.php
@@ -4,14 +4,43 @@
+    $this->database = $database;

    $this->ucWishlistManager = $ucwishlist_manager; 

  43. +++ b/src/Form/UCWishlistAdminDeleteForm.php
@@ -69,9 +95,9 @@ class UCWishlistAdminDeleteForm extends ConfirmFormBase {
+    $database = new UcWishlistManager(\Drupal::database());

    There's no need to statically instantiate the object of the service. It will be available as $this->wishlistManager because it's injected above in the constructor. 

  44. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,195 @@
+ * @file contains user wishlist settings to extend the

    It's the comment/description of the Class, so remove @file from the comment. 

  45. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,195 @@
+    $account = \Drupal::currentUser();

    Inject the currentUser service using DI in the form. 

  46. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,195 @@
+      drupal_set_message(t('Could not find the specified wish list.'), 'error');

    use $this->t 

  47. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,195 @@
+      drupal_set_message(t('You do not have permission to edit this wish list.'), 'error');

    use $this->t 

  48. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,195 @@
+    uc_wishlist_update_wishlist($form_state->getValues['wid'], $form_state->getValues['title'], $expiration, (object) $address, $private);

    Where's this function? 

  49. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,195 @@
+    drupal_set_message(t('Your wish list has been updated.'));

    use $this->t 

  50. +++ b/src/Form/WishlistSearchForm.php
@@ -33,20 +22,8 @@ class WishlistSearchForm extends FormBase {
     $account = \Drupal::currentUser();

    Inject the current user service in this class. 

  51. +++ b/src/Form/WishlistViewForm.php
@@ -7,21 +7,40 @@ use Drupal\Core\Form\FormBase;
   protected $database;

    Rename this property to $ucwishlistManager 

  52. +++ b/src/Form/WishlistViewForm.php
@@ -7,21 +7,40 @@ use Drupal\Core\Form\FormBase;
+  public function __construct(UcWishlistManager $database) {

    UcWishlistManager $ucwishlist_manager 

  53. +++ b/src/Form/WishlistViewForm.php
@@ -7,21 +7,40 @@ use Drupal\Core\Form\FormBase;
+    $this->database = $database;

    $this->ucwishlistManager = $ucwishlist_manager; 

  54. +++ b/src/Form/WishlistViewForm.php
@@ -276,16 +291,13 @@ class WishlistViewForm extends FormBase {
+        $database = new UcWishlistManager(\Drupal::database());

    you could access this service using $this->ucwishlistManager 

  55. +++ b/uc_wishlist.module
@@ -215,7 +219,7 @@ function uc_wishlist_create_wishlist($title = NULL) {
+  $db = \Drupal::service('uc_wishlist.manager');

    rename variable name to $wishlist_manager instead of $db 

  56. +++ b/uc_wishlist.module
@@ -228,7 +232,7 @@ function uc_wishlist_create_wishlist($title = NULL) {
+  $db = \Drupal::service('uc_wishlist.manager');

    rename variable name to $wishlist_manager 

  57. +++ b/uc_wishlist.module
@@ -327,7 +331,7 @@ function uc_wishlist_add_item($nid, $qty = 1, $data = NULL, $wid = NULL, $msg =
+  $db = \Drupal::service('uc_wishlist.manager');

    rename variable name to $wishlist_manager 

  58. +++ b/uc_wishlist.module
@@ -439,7 +442,7 @@ function uc_wishlist_get_wid($uid = NULL) {
+  $db = \Drupal::service('uc_wishlist.manager');

    rename variable name to $wishlist_manager 

  59. +++ b/uc_wishlist.module
@@ -481,7 +484,7 @@ function uc_wishlist_load($wid) {
+  $dbQuery = \Drupal::service('uc_wishlist.manager');

    rename variable name to $wishlist_manager 

  60. +++ b/uc_wishlist.module
@@ -500,7 +503,7 @@ function uc_wishlist_get_contents($wid = NULL) {
+  $database = \Drupal::service('uc_wishlist.manager');

    rename variable name to $wishlist_manager

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented

@naveenvalecha,

Thanks. Working on the final part.

Will submit fresh patch shortly.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented
FileSize
2904915-final-fix-3.patch45.87 KB

@naveenvalecha,

Addressed the issues listed in #40.

Please review.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented
Status: Needs work » Needs review
FileSize
2904915-final-fix-3.patch45.87 KB
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented

@naveenvalecha,

Please review.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work 
  1. +++ b/src/Controller/UCWishlistController.php
@@ -6,17 +6,30 @@ use Drupal\Core\Controller\ControllerBase;
+   *

    Add the description/comment 

  2. +++ b/src/Controller/UCWishlistController.php
@@ -6,17 +6,30 @@ use Drupal\Core\Controller\ControllerBase;
    *

    Add the description and add the comment for @param 

  3. +++ b/src/Controller/UCWishlistController.php
@@ -6,17 +6,30 @@ use Drupal\Core\Controller\ControllerBase;
+   *

    Add the description here 

  4. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,311 @@
+    $this->connection->query("SELECT * FROM {uc_wishlist_products} WHERE nid = :pid AND wid = :wid", [':pid'=>$pid,':wid'=>$wid]);

    Add the return statement as well i.e. return $this->connection 

  5. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,311 @@
+  public function remove_item($wpid) {

    Method name is not per coding standards. It should be removeItem 

  6. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,311 @@
+    $this->connection->delete('uc_wishlist_products')->condition('wpid', $wpid)->execute();

    same as above. add the return as well. 

  7. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,311 @@
+  public function remove_product($pid) {

    Method name is not per coding standards. it should be removeProduct 

  8. +++ b/src/Database/UcWishlistManager.php
@@ -0,0 +1,311 @@
+    $this->connection->delete('uc_wishlist_products')->condition('nid',$pid)->execute();

    same as above. 

  9. +++ b/src/Form/UCWishlistAdminDeleteForm.php
@@ -4,14 +4,43 @@
+  protected $ucWishlistManager;

    use the common pattern across the code either use the wishlistManager/ucWishlistManager In above controller you have defined the property name $wishlistManager 

  10. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,217 @@
+   * @var AccountInterface $account

    Typehint the class with full namespace i.e. \Drupal\Core\Session\AccountInterface 

  11. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,217 @@
+  /**

    Add the description i.e. Constructs an UserWishlistSettingsForm class. 

  12. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,217 @@
+    $config = $this->config('uc_product.settings');

    are you sure it's uc_product.settings or uc_wishlist.settings 

  13. +++ b/src/Form/UserWishlistSettingsForm.php
@@ -0,0 +1,217 @@
+    $account = $this->account;

    Is this local variable requied. can't $this->account gets used everywhere? 

  14. +++ b/src/Form/WishlistSearchForm.php
@@ -2,23 +2,35 @@
+   * @var AccountInterface $account

    Typehint it with full namespace. 

  15. +++ b/src/Form/WishlistSearchForm.php
@@ -32,21 +44,9 @@ class WishlistSearchForm extends FormBase {
+    $account = $this->account;

    Is this local variable requied. can't $this->account gets used everywhere? 

  16. +++ b/src/Form/WishlistViewForm.php
@@ -276,16 +291,13 @@ class WishlistViewForm extends FormBase {
+        $database = $this->ucwishlistManager;

    Is this local variable required? can't $this->ucwishlistManager be used? 

  17. +++ b/uc_wishlist.module
@@ -327,9 +331,9 @@ function uc_wishlist_add_item($nid, $qty = 1, $data = NULL, $wid = NULL, $msg =
+  $item = $wishlist_manager->getWishlistItem($wid,$nid,$data);//$result->fetchObject();

    Does this commented code really require? 

  18. +++ b/uc_wishlist.module
@@ -439,8 +442,8 @@ function uc_wishlist_get_wid($uid = NULL) {
+  return $wishlist_manager->getWishlistIdByUser($uid);//db_query("SELECT wid FROM {uc_wishlists} WHERE uid = :uid", array(':uid' => $uid))->fetchField();

    Does this commented code require? 

  19. +++ b/uc_wishlist.module
@@ -481,8 +484,8 @@ function uc_wishlist_load($wid) {
+  $result = $wishlist_manager->getWishlist($wid);//db_query("SELECT * FROM {uc_wishlists} WHERE wid = :wid", array(':wid' => $wid));

    Does this commented code require?

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented
Status: Needs work » Needs review
FileSize
2904915-final-fix-4.patch46.25 KB

@naveenvalecha,

Addressed all the above listed issues.

Please review.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work
FileSize
Screen Shot 2017-09-05 at 6.17.09 PM.png108.56 KB

phew :( you didn't test the code before posting the patch.I'm getting WSOD while enabling the module. See the screenshot attached. Let me know if you need any help.

P.S.: I'm tired of requesting this thing again and again in the several issues to test the code before posting the patch because of untested code waste of time of both parties.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Issue summary: View changes
Related issues: +#2889425: Porting Uc wishlist module to Drupal 8 | Phase 1, +#2880447: Porting Uc wishlist module to Drupal 8 | Phase 2

Updating Issue summary

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented
Status: Needs work » Needs review
FileSize
2904915-final-fix-5.patch46.25 KB
naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
FileSize
2904915-50.patch79.3 KB
interdiff-2904915-49-50.txt50.12 KB

Yay! This is super neat!
A lot of coding standards are missing. See https://www.drupal.org/pift-ci-job/756133
Fixed some coding standards by running phpcbf on it phpcbf --standard=Drupal uc_wishlist/

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work

Coding standards messages are down from 471(https://www.drupal.org/pift-ci-job/756133) to 94(https://www.drupal.org/pift-ci-job/756160)
Fix these coding standards warnings https://www.drupal.org/pift-ci-job/756160
Once this will be done. I'll thoroughly review and test the patch.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented

@naveenvalecha,

On it.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs work » Fixed

Committed and pushed to 8.x-1.x because the patch is gonna big.
Still, there are a lot of issues to address. Filed a follow-up for that #2907204: UC Wishlist review - Part 2

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 commented

Thanks. Working on the remaining issues.

Status: Fixed » Closed (fixed)

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