Convert this page callback to a new-style FormInterface implementation, using the instructions on http://drupal.org/node/1800686.

Files: 
CommentFileSizeAuthor
#24 drupal-aggregator_form_opml-1963410-24.patch17.98 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,658 pass(es).
[ View ]
#21 drupal-aggregator_form_opml-1963410-21.patch17.96 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,320 pass(es).
[ View ]
#20 drupal-aggregator_form_opml-1963410-20.patch17.95 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#19 drupal-aggregator_form_opml-1963410-19.patch17.94 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,528 pass(es).
[ View ]
#16 aggregator-convert_aggregator_form_opml-1963410-16.patch14.82 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 54,254 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#16 interdiff.txt2.48 KBmtift
#11 aggregator-convert_aggregator_form_opml-1963410-11.patch14.54 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 54,168 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#11 interdiff.txt4.36 KBmtift
#8 aggregator-convert_aggregator_form_opml-1963410-8.patch12.94 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 54,086 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#8 interdiff.txt7.69 KBmtift
#5 aggregator-convert_aggregator_form_opml-1963410-5.patch12.89 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 54,005 pass(es), 35 fail(s), and 2 exception(s).
[ View ]
#5 interdiff.txt581 bytesmtift
#3 aggregator-convert_aggregator_form_opml-1963410-3.patch12.89 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 53,977 pass(es), 35 fail(s), and 2 exception(s).
[ View ]

Comments

Title:Convert aggregator_form_opml to a FormInterface implementation and routing definition.Convert aggregator_form_opml to a FormInterface implementation and routing definition
Issue tags:-WSCCI

Assigned:Unassigned» mtift

I'll take this one.

Status:Active» Needs review
StatusFileSize
new12.89 KB
FAILED: [[SimpleTest]]: [MySQL] 53,977 pass(es), 35 fail(s), and 2 exception(s).
[ View ]

Here's my first attempt.

Status:Needs review» Needs work

The last submitted patch, aggregator-convert_aggregator_form_opml-1963410-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new581 bytes
new12.89 KB
FAILED: [[SimpleTest]]: [MySQL] 54,005 pass(es), 35 fail(s), and 2 exception(s).
[ View ]

Fixed "route_name" typo

Status:Needs review» Needs work

The last submitted patch, aggregator-convert_aggregator_form_opml-1963410-5.patch, failed testing.

Hi, thanks for picking this up!

Looks good so far, some notes:

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -4,3 +4,10 @@ aggregator_feed_items_delete:
+add_opml:

lets call the route aggregator_opml_add so it is consistent with the rest routes there and also correctly prefixed

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -4,3 +4,10 @@ aggregator_feed_items_delete:
+    _content: '\Drupal\aggregator\Form\FeedAddOpml'

it should be _form and lets call the class OpmlAdd or OpmlFeedAdd so its inline with FeedItemsDelete )verb last)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedAddOpml.phpundefined
@@ -0,0 +1,172 @@
+      $response = drupal_http_request($form_state['values']['remote']);

this is being converted to guzzle on #1862524: Convert drupal_http_request usage in aggregator.module to Guzzle so when it is in (rtbc now) we can inject guzzle as well:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedAddOpml.phpundefined
@@ -0,0 +1,172 @@
+    $feeds = _aggregator_parse_opml($data);

opened #1963540: Move _aggregator_parse_opml to the parser plugin for this, so we can inject this too, you can leave it as is for now:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedAddOpml.phpundefined
@@ -0,0 +1,172 @@
+      $result = db_query("SELECT title, url FROM {aggregator_feed} WHERE title = :title OR url = :url", array(':title' => $feed['title'], ':url' => $feed['url']));

should get rid of db_query and use $this->database->query instead

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedAddOpml.phpundefined
@@ -0,0 +1,172 @@
+      $new_feed = entity_create('aggregator_feed', array(

Instead of using entity_create here, lets inject the entity manager and do what http://api.drupal.org/api/drupal/core!includes!entity.inc/function/entit... does:) get the storage and call create on it

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedAddOpml.phpundefined
@@ -0,0 +1,172 @@
+            'title' => $feed['title'],
+            'url' => $feed['url'],
+            'refresh' => $form_state['values']['refresh'],
+            'block' => $form_state['values']['block'],
+            ));

indentation is off

Status:Needs work» Needs review
StatusFileSize
new7.69 KB
new12.94 KB
FAILED: [[SimpleTest]]: [MySQL] 54,086 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

Thanks for the review, @rootatwc! The attached patch covers most of the comments in #7, except for the drupal_http_request() call.

Status:Needs review» Needs work

The last submitted patch, aggregator-convert_aggregator_form_opml-1963410-8.patch, failed testing.

the guzzle patch just committed so together with entity manager they should now be injected in the class

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/OpmlFeedAdd.phpundefined
@@ -0,0 +1,174 @@
+      $new_feed = drupal_container()->get('plugin.manager.entity')

Not what i meant above.the plugin manager entity should be passed in the __construct with database, and should be stored in $this->entityManager..then no need to use drupal_container()

Status:Needs work» Needs review
StatusFileSize
new4.36 KB
new14.54 KB
FAILED: [[SimpleTest]]: [MySQL] 54,168 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

I got a bit confused with my re-roll, and ended up doing a straight diff rather than a git diff. I also felt a little odd blowing away the changes from #1862524: Convert drupal_http_request usage in aggregator.module to Guzzle and putting them right back into this patch.

great! entity plugin manager is injected now:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/OpmlFeedAdd.phpundefined
@@ -0,0 +1,198 @@
+        $response = Drupal::httpClient()

This should be injected as well, together with database and entity manager:)
it should be stored in $this->httpClient

Status:Needs review» Needs work

The last submitted patch, aggregator-convert_aggregator_form_opml-1963410-11.patch, failed testing.

hmm...

#1963540: Move _aggregator_parse_opml to the parser plugin would remove this entirely. Creating instead a different bundle with an OPML pipeline.

yeah but i am not really sure whether #1966070: Make aggregator feeds bundleable - move plugins configuration per bundle makes it in d8, so this is a really good step

Status:Needs work» Needs review
StatusFileSize
new2.48 KB
new14.82 KB
FAILED: [[SimpleTest]]: [MySQL] 54,254 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

So like this?

Status:Needs review» Needs work

The last submitted patch, aggregator-convert_aggregator_form_opml-1963410-16.patch, failed testing.

yes this looks great now, thanks
not sure why tests fail though, i should check manually once i find some time

Status:Needs work» Needs review
StatusFileSize
new17.94 KB
PASSED: [[SimpleTest]]: [MySQL] 54,528 pass(es).
[ View ]

tests failed because _aggregator_parse_opml was in the admin.inc which was not loaded.
I moved it to a protected method to the controller with a todo above it.
Also injected the entityQuery and converted a query to use it instead:)

StatusFileSize
new17.95 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

rerolled for conflict with multiple uploader patch, yay:)

StatusFileSize
new17.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,320 pass(es).
[ View ]

uhm, wrong patch above, does not contain the addition in file_save_upload in the Controller

This looks good to me, waiting for the bot

Status:Needs review» Reviewed & tested by the community

I can't find anything wrong with #21 :)

StatusFileSize
new17.98 KB
PASSED: [[SimpleTest]]: [MySQL] 55,658 pass(es).
[ View ]

Assigned:mtift» Unassigned
Status:Reviewed & tested by the community» Fixed

Committed a1d4967 and pushed to 8.x. Thanks!

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