Files: 
CommentFileSizeAuthor
#29 2046303-29-aggregator-un-use-d.patch688 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,176 pass(es).
[ View ]
#22 2046303-aggregator-category-form-22.patch27.8 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,924 pass(es).
[ View ]
#22 interdiff.txt776 byteskim.pepper
#21 2046303-aggregator-category-form-21.patch27.69 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#21 interdiff.txt683 byteskim.pepper
#19 2046303-aggregator-category-form-19.patch27.41 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,883 pass(es).
[ View ]
#19 interdiff.txt5.89 KBkim.pepper
#16 2046303-aggregator-category-form-16.patch27.16 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,126 pass(es).
[ View ]
#16 interdiff.txt4.22 KBkim.pepper
#12 2046303-aggregator-category-form-12.patch27.01 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,879 pass(es).
[ View ]
#12 interdiff.txt3.42 KBkim.pepper
#7 2046303-aggregator-category-form-7.patch26.49 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 54,801 pass(es), 99 fail(s), and 111 exception(s).
[ View ]
#7 interdiff.txt9.25 KBkim.pepper
#4 2046303-aggregator-category-form-4.patch26.26 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,234 pass(es).
[ View ]
#4 interdiff.txt2.72 KBkim.pepper
#3 2046303-aggregator-category-form-3.patch26.89 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,212 pass(es).
[ View ]
#3 interdiff.txt841 byteskim.pepper
#1 2046303-aggregator-category-form-1.patch26.29 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,836 pass(es), 17 fail(s), and 6 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new26.29 KB
FAILED: [[SimpleTest]]: [MySQL] 56,836 pass(es), 17 fail(s), and 6 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new841 bytes
new26.89 KB
PASSED: [[SimpleTest]]: [MySQL] 57,212 pass(es).
[ View ]

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

StatusFileSize
new2.72 KB
new26.26 KB
PASSED: [[SimpleTest]]: [MySQL] 57,234 pass(es).
[ View ]

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

Issue summary:View changes

Added link to taxonomy issue

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

Status:Needs work» Needs review
StatusFileSize
new9.25 KB
new26.49 KB
FAILED: [[SimpleTest]]: [MySQL] 54,801 pass(es), 99 fail(s), and 111 exception(s).
[ View ]

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.

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.

+++ 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?

Status:Needs work» Needs review
Issue tags:-WSCCI-conversion
StatusFileSize
new3.42 KB
new27.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,879 pass(es).
[ View ]

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

Issue tags:+WSCCI-conversion

Tag monster.

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

+++ 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

Status:Needs work» Needs review
StatusFileSize
new4.22 KB
new27.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,126 pass(es).
[ View ]

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?

double-post

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

Status:Needs work» Needs review
StatusFileSize
new5.89 KB
new27.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,883 pass(es).
[ View ]

Thanks again.

Here are the fixes for #18.

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:)

StatusFileSize
new683 bytes
new27.69 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Kills legacy sql as per #20.

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

StatusFileSize
new776 bytes
new27.8 KB
PASSED: [[SimpleTest]]: [MySQL] 57,924 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

thank you!

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.

Issue tags:-API change

Not really. Before the patch:

<?php
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

<?php
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:)

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

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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

Status:Fixed» Needs review
StatusFileSize
new688 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,176 pass(es).
[ View ]
  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.

Status:Needs review» Reviewed & tested by the community

Nice catch.

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.

Issue summary:View changes

Added related issues