Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Status: Active » Needs review
FileSize
26.29 KB

This is quite a bit more than a form conversion.

  1. It includes a new CategoryStorage service for crud operations on categories.
  2. It moves a lot of the extra menu linking that was in aggregator_save_category and puts it in the controller
  3. It adds a confirm delete form for deleting categories
  4. It adds extra delete link to category operation on the overview page

Status: Needs review » Needs work

The last submitted patch, 2046303-aggregator-category-form-1.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
841 bytes
26.89 KB

Removed leftover references to non-existant aggregator.admin.inc file.

kim.pepper’s picture

No need for entityManager in CategoryAdminForm any more, so removed.

kim.pepper’s picture

Issue summary: View changes

Added link to taxonomy issue

ParisLiakos’s picture

ParisLiakos’s picture

Status: Needs review » Needs work

Thanks, looks good so far

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.phpundefined
@@ -0,0 +1,80 @@
+<?php
+/**

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -0,0 +1,52 @@
+<?php
+/**

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,186 @@
+<?php
+/**

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryDeleteForm.phpundefined
@@ -0,0 +1,188 @@
+<?php
+/**

Needs a newline

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -44,6 +44,8 @@ function aggregator_help($path, $arg) {
+    case 'admin/config/services/aggregator/delete':
+      return;

not sure whats that? is there such a path?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.phpundefined
@@ -0,0 +1,80 @@
+  }
+
+
+}

One less newline:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -0,0 +1,52 @@
+  public function save(array $category);
...
+  public function update(array $category);

Any reason we couldnt have those methods merged in a single save() method?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,186 @@
+    $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Save'));
...
+      $form['actions']['delete'] = array('#type' => 'submit', '#value' => t('Delete'));
+      $form['cid'] = array('#type' => 'hidden', '#value' => $category->cid);

can we have those in multiple lines?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,186 @@
+        $category = $this->database->query("SELECT cid FROM {aggregator_category} WHERE title = :title AND cid <> :cid", array(':title' => $form_state['values']['title'], ':cid' => $form_state['values']['cid']))->fetchObject();
...
+        $category = $this->database->query("SELECT cid FROM {aggregator_category} WHERE title = :title", array(':title' => $form_state['values']['title']))->fetchObject();

Could we have methods for those too?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,186 @@
+      $form_state['redirect'] = arg(0) == 'admin' ? 'admin/config/services/aggregator/' : 'aggregator/categories/' . $cid;

lets use the Request object and not arg()

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,186 @@
+      \Drupal::service('plugin.manager.block')->clearCachedDefinitions();

inject this?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryDeleteForm.phpundefined
@@ -0,0 +1,188 @@
+    $form_state['redirect'] = arg(0) == 'admin' ? 'admin/config/services/aggregator' : 'aggregator';

same here. lets use the Request

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
26.49 KB

Thanks for the review @ParisLiakos.

  1. Added newlines before file docblocks
  2. Removed unneeded help path case
  3. Remove extra newline before class closing parenthesis

Any reason we couldnt have those methods merged in a single save() method?

I would prefer to keep them separate as insert has a return value of the new id, and they are running completely different queries in the implementation. I'd like to avoid passing in $op params wherever possible.

Could we have methods for those too?

I've added a new method isUnique() on the CategoryStorageControllerInterface, that takes an optional $cid param to exclude for checking for uniqueness if we are renaming an existing category.

lets use the Request object and not arg()

Injecting Request in both forms now via buildForm().

\Drupal::service('plugin.manager.block')->clearCachedDefinitions();
Inject this?

I wasn't sure this was ok, since we don't know if the service exists? We are checking if the module is enabled first.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion

The last submitted patch, 2046303-aggregator-category-form-7.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +FormInterface, +WSCCI-conversion

The last submitted patch, 2046303-aggregator-category-form-7.patch, failed testing.

ParisLiakos’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.phpundefined
@@ -76,5 +77,18 @@ public function delete($cid) {
+  public function isUnique($title, $cid = NULL) {

isUnique sounds great thanks:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.phpundefined
@@ -76,5 +77,18 @@ public function delete($cid) {
+    $rows = $query->execute()->fetchColumn();

fetchCol?

I wasn't sure this was ok, since we don't know if the service exists? We are checking if the module is enabled first.

Well, you could make the constructor argument optional, and perform the same check with the module handler in the create() method, right?

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI-conversion
FileSize
3.42 KB
27.01 KB

Fixes fetchCol() and conditionally injects the BlockManager as suggested in #11.

kim.pepper’s picture

Issue tags: +WSCCI-conversion

Tag monster.

ParisLiakos’s picture

Status: Needs review » Needs work

well, looks great..two more nitpicks and sth else which if its too much of work we could leave for a followup

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.phpundefined
@@ -0,0 +1,94 @@
+   * @param Connection $database

Connection needs the full namespace

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -0,0 +1,69 @@
+   * @return array|null
+   *   An array of category properties.
+   */
+  public function load($cid);
...
+   * @return int
+   *   The new category ID.
+   */
+  public function save(array $category);
...
+  public function update(array $category);

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,225 @@
+    $category = (object) $this->categoryStorageController->load($cid);

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryDeleteForm.phpundefined
@@ -0,0 +1,200 @@
+    $category = (object) $this->categoryStorageController->load($cid);

how much more work would it be, to make those arrays an stdClass object instead, so we avoid those (object) conversions..lets standarise to stdClass for now..till they become taxonomy terms

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -0,0 +1,69 @@
+  public function isUnique($title, $cid = NULL);
+}

needs a newline in between

ParisLiakos’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageController.phpundefined
@@ -0,0 +1,94 @@
+  public function load($cid) {
+    return $this->database->query("SELECT cid, title, description, block FROM {aggregator_category} WHERE cid = :cid", array(':cid' => $cid))->fetchAssoc();
+  }

also..i was now checking aggregator_category_load() and well, i think that this function should be marked deprecated and just be an one line wrapper for
CategoryStorageControllerInterface::load()

so i noticed that currently we do fetchObject() not fetchAssoc(). also the query does SELECT * which i like better, because it means, it will load any additional fields added by hook_schema_alter.

Finally this method should similarly take care of static cache, probably storing loaded category stdClasses in a class property, not drupal_static ofc

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
27.16 KB

Thanks for the review.

This takes care of all changes in #14 and #15 except for:

Finally this method should similarly take care of static cache, probably storing loaded category stdClasses in a class property, not drupal_static ofc

Can this be a follow up?

kim.pepper’s picture

double-post

ParisLiakos’s picture

Status: Needs review » Needs work

Thank you:)

+++ b/core/modules/aggregator/aggregator.services.ymlundefined
@@ -13,3 +13,6 @@ services:
+    arguments: ['@database', '@plugin.manager.entity', '@module_handler']

so you got rid of the unused "use" statements but we should fix the arguments here:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -0,0 +1,70 @@
+   * @return stdClass|null
+   *   An array of category properties.

you updated the @return but not the description:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -0,0 +1,70 @@
+  public function save(array $category);
...
+  public function update(array $category);

when i was asking about standarizing to stdClass i meant those too:) before it was consistent to array, but now load() gives you an object save and update take an array.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.phpundefined
@@ -0,0 +1,225 @@
+      $this->categoryStorageController->update($form_state['values']);
...
+    $cid = $this->categoryStorageController->save($form_state['values']);

so we typecast (object) on $form_state['values'] before passing to storage controller.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryDeleteForm.phpundefined
@@ -0,0 +1,200 @@
+   * @var
+   */
+  protected $request;

oops i missed that in the previous review

Can this be a follow up?

Well, if we want to make aggregator_category_load() deprecated, we need to make it one-liner wrapper:) i dont think it is committable with keeping the same logic in two places in this point of release cycle

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
5.89 KB
27.41 KB

Thanks again.

Here are the fixes for #18.

ParisLiakos’s picture

besides killing the legacy sql in the deprecated function:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/CategoryStorageControllerInterface.phpundefined
@@ -19,28 +19,28 @@
-  public function save(array $category);
+  public function save($category);
...
-  public function update(array $category);
+  public function update($category);

we could still typehint this with \stdClass i guess:)

kim.pepper’s picture

Kills legacy sql as per #20.

Agreed with ParisLiakos in IRC to leave out stdClass type hinting.

kim.pepper’s picture

Got confused with the incorrect docblock comments on aggregator_category_load(). We actually return an stdClass and not an associative array. Updated the function and the comments.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thank you!

tstoeckler’s picture

Issue tags: +API change

So the last patch actually makes this an API change, technically. For a non-existing $cid aggregator_category_load() now returns NULL, where it returned an empty array before. Because the previous behavior wasn't very semantic anyway I think it's OK do make this change, but this needs committer approval. It would also be possible to retain the current behavior but in this case, I really don't see why that would be necessary.

ParisLiakos’s picture

Issue tags: -API change

Not really. Before the patch:

function aggregator_category_load($cid) {
  $categories = &drupal_static(__FUNCTION__);
  if (!isset($categories[$cid])) {
    $categories[$cid] = db_query('SELECT * FROM {aggregator_category} WHERE cid = :cid', array(':cid' => $cid))->fetchObject();
  }

  return $categories[$cid];
}

After the patch

function aggregator_category_load($cid) {
  return Drupal::service('aggregator.category.storage')->load($cid);
}

// the load() method maintains same behavior:
//
  public function load($cid) {
    if (!isset($this->categories[$cid])) {
      $this->categories[$cid] = $this->database->query("SELECT * FROM {aggregator_category} WHERE cid = :cid", array(':cid' => $cid))->fetchObject();
    }
    return $this->categories[$cid];
  }
//

Which is exact the same behavior

Its the docs that were inaccurate before, which @kim.pepper fixed:)

tstoeckler’s picture

Ahh, I see. How naive of me to trust our API documentation. Sorry for the noise and thanks for clearing that up!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

ParisLiakos’s picture

followup for forgotten aggregator_save_category() :)
#2068393: Mark aggregator_save_category() as deprecated

tstoeckler’s picture

Status: Fixed » Needs review
FileSize
688 bytes
  1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.php
    @@ -0,0 +1,225 @@
    +use Drupal\Core\Entity\EntityManager;
    

    This seems to be unused.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Nice catch.

ParisLiakos’s picture

Status: Reviewed & tested by the community » Fixed

lets take this to the followup #2068393: Mark aggregator_save_category() as deprecated shall we?
its already fixed there.

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

Anonymous’s picture

Issue summary: View changes

Added related issues