Problem/Motivation
As part of #1971384: [META] Convert page callbacks to controllers we are converting the aggregator form callbacks to FormInterface. The two forms at /aggregator/categories/{cid}/categorize and /aggregator/sources/{aggregator_feed}/categorize share much of the same code, only the list of feed items is filtered differently: one by the source feed it comes from, and one by category.
The existing code uses form callbacks that wrap page callbacks, that then call common protected code that either builds a render array or a form array depending on a switch... It gets confusing.
Proposed resolution
Break out the two forms into a FormInterface implementations with common abstract base class that has all the common code. The left-over procedural code will be handled with the controller conversions in #1974408: Convert aggregator_page_source() to a Controller
Remaining tasks
Follow ups:
User interface changes
None.
API changes
None.
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Blocks the following issues
#1974394: Convert aggregator_page_category() to a Controller
#1974408: Convert aggregator_page_source() to a Controller
Related Issues
#2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
#2046303: Convert aggregator_form_category to FormInterface
#2003642: Convert aggregator_page_source_form to the new form interface
Follow ups
#2063671: Use the aggregator category storage in AggregatorCategorizeFormBase
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff.txt | 2.17 KB | kim.pepper |
#52 | 2040199-page-category-form-52.patch | 14.93 KB | kim.pepper |
#48 | 2040199-page-category-form-48.patch | 14.87 KB | kim.pepper |
#48 | interdiff.txt | 3.09 KB | kim.pepper |
#47 | 2040199-page-category-form-47.patch | 15.02 KB | kim.pepper |
Comments
Comment #1
larowlanComment #2
afeijohere is my initial code
Thanks a lot for timplunkett, Crell and larowlan (so far :] )
AggregatorCategoryForm.php need a lot of work, it is a copy of ActionAdminManagerForm.php
Comment #3
afeijoso far code attached
it has this error:
Fatal error: Call to undefined method Drupal\aggregator\Form\AggregatorCategoryForm::pageCategory() in /www/web/drupal/core/modules/aggregator/lib/Drupal/aggregator/Form/AggregatorCategoryForm.php on line 54
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedWelcome to aggregator's pages.inc!
this is a
_form
callback and should be converted as such. But, in order to figure out what happens here you need to check _aggregator_page_list the very first conditional...so, here a pretty hacky thing is done:)
You should copy the logic from
aggregator_categorize_items
andaggregator_categorize_items_submit
after doing theaggregator_load_feed_items
call.Ignore everything else.
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedpostponed #2003642: Convert aggregator_page_source_form to the new form interface on this
Comment #6
afeijoHere it is my latest changes
the categories page now loads, but with just 1 button. It is not usable. And I guess the original code before my changes isnt good ether.
http://drupal8.dev/aggregator/categories/1/categorize
if I reset --hard it, that page display 3 notices !?
2 a.m., not a good time to confirm it :) tomorrow I will do a fresh review
thanks once more larowlan
Comment #7
Gábor HojtsyDoes not seem to be related to multilingual at all.
Comment #8
larowlanSo this passes all aggregator tests locally.
But it seems to include removing aggegator_page_category too.
Is it possible that your patch from above contains extra cruft @aFeijo?
Also added #2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper
Comment #9
larowlanDoes this belong in this patch?
can we add a reference to https://drupal.org/node/2043581
Can we add a @todo for https://drupal.org/node/15266
We should explicity inject the render controller and not the entity manager.
Comment #10
afeijook larowlan,
Uploading the patch with the changes we've talked
Comment #11
larowlanCan we add a reference to https://drupal.org/node/15266 and a trailing .
this is being handled in https://drupal.org/node/1974394 and needs to be removed from this patch
we can remove 'is refactored' now
needs indent
No longer needed that I can see
These comments aren't consistent with the other others (which don't use Stores), should we make them consistent?
Should be removed
Comment #12
ygerasimov CreditAttribution: ygerasimov commentedThis should be out of this patch as it is not related to the form.
This is also not related to the form.
The issue that addresses aggregator/categories/{category} is #1974394: Convert aggregator_page_category() to a Controller. Lets keep these two issues separate.
Comment #13
afeijook friends, new patch
Comment #15
kim.pepperRe-roll.
I've created a new CategoryStorageController for categories in #2046303: Convert aggregator_form_category to FormInterface. Waiting for this issue to go in first.
Comment #15.0
kim.pepperrelated issues
Comment #17
kim.pepperThis fixes a couple of the failures in #15 and adds some documentation fixes along the way.
Comment #18
kim.pepperOops. Wrong interdiff. This one is correct.
Comment #19
larowlanMore than 80 chars and not a well-structured sentence. Also lower case r. Perhaps
Remove once aggregator_load_feed_items() is .... See https://drupal.org/node/2043581.
replace the ... with whatever its going to be (eg method on storage controller).Can we add this back (nitpick)
Can we get this in alpha order? (nitpick)
Can we change this to a hard dependency on a EntityRenderControllerInterface? Then move the ->getRenderController() to the ::create method? Then the dependency is more transparent.
We're injecting the whole factory for just one config item, I think its preferred to just grab the single Config item (see comments from @alexpott on #1951268-61: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service)
Comment #20
kim.pepperThanks @larowlan!
This includes fixes as per #19.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedthis is more than 80 chars..also i think we should just leave it alone, it reduces the possibilities to break other patches, and also not so much in scope here
more out of scope changes.
lets not touch the Controller at all here, since its not needed
if we only need this particular config, why inject the whole factory?
Lets inject directly just the aggregator.settings config
+1 for the @todo
multiple lines please
Comment #22
kim.pepperThanks for the review @ParisLiakos.
This fixes all issues raised in #21.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedseems ready, but #2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper went in, so we need to account for the changes there
Comment #24
kim.pepperRefactored to use ItemStorageController. This allows us to remove loading aggregator.pages.inc and therefore the module loader too.
Comment #25
afeijoGlad to see this issue going!
Thanks for the help, guys
I have 2 hard weeks with sickness upon my family and myself. Damn winter :)
Comment #27
kim.pepperLocaleUpdateCronTest.php failed. Re-testing.
Comment #28
kim.pepper#24: 2040199-page-category-form-24.patch queued for re-testing.
Comment #30
kim.pepper"Failed to write configuration file". re-testing.
Comment #31
kim.pepper#24: 2040199-page-category-form-24.patch queued for re-testing.
Comment #33
kim.pepper#24: 2040199-page-category-form-24.patch queued for re-testing.
Comment #33.0
kim.peppermore related issues
Comment #34
kim.pepperThis patch combines
aggregator_page_source_form()
andaggregator_page_category_form()
into a new base form class that takes care of the common form building functionality provided byaggregator_categorize_items()
and two sub-classes.As
aggregator_categorize_items()
is no longer called inside_aggregator_page_list()
, this make it much easier to move the remaining common render array building functionality into AggregatorController for #1974394: Convert aggregator_page_category() to a Controller and #1974408: Convert aggregator_page_source() to a Controller.Marking #2003642: Convert aggregator_page_source_form to the new form interface as a duplicate of this issue since it combines the two.
Comment #34.0
kim.peppermore related issues
Comment #35
dawehnerSome manual testing on simplytest.me worked totally perfect, here are some comments + one nitpick.
Just as a site-node: We can't convert them really easy yet to local task plugins...
One of these two lines is wrong, guess which of those.
Fuck yeah!
a db query in a foreach always sounds wrong ... maybe open a followup?
Comment #35.0
dawehneradded issues blocked by this
Comment #36
kim.pepperThanks dawehner!
I've fixed the typo in comments and created a follow up here #2063671: Use the aggregator category storage in AggregatorCategorizeFormBase
Comment #37
dawehnerThank you!
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedlooks awesome, +1 for the followup. re-titling to reflect the scope change in #34
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #40
effulgentsia CreditAttribution: effulgentsia commented#1974394: Convert aggregator_page_category() to a Controller is postponed on this.
Comment #41
tim.plunkett#2059245: Add a FormBase class containing useful methods should be going in soon, FYI.
What's this for?
Why isn't this just buildForm(), and these could call parent::buildForm()?
Or, even better.
ImageEffectFormBase has a similar need to this, but the base class specifies
abstract protected function prepareImageEffect($image_effect);
, and just calls that in buildForm in the base, so each child is required to specify its effect loading.These should say what they're for, like aggregatorItemStorage
This should be typehinted
Comment #42
kim.pepperThanks Tim.
This fixes 1) 3) and 4) from #41
For 2) We have 2 different form routes that take different parameters, so need buildForm() in our subclasses. That means we can't have one buildForm in the abstract base class.
If accept that, then I don't think it makes sense to have a buildForm() in the base class that doesn't actually get called by a route like all other buildForm() methods do.
Comment #43
kim.pepperAlso, not sure what FormBase will give us. We don't need anything that is provided by FormBase.
Comment #44
jibranLet's move this to routes.yml
Should we fix this using Local tasks are provided by annotated plugins implementing LocalTaskInterface instead of type MENU_LOCAL_TASK in hook_menu()? Or should we wait for #2050919: Replace local task plugin discovery with YamlDiscovery
Any reason why this is not a list controller?
Because of
FormBase
we can remove all the boilerplate code.Comment #45
kim.pepper1) We need to have a title on our tab links. Moving to the route definition doesn't fix that.
2) I hope we don't need hold this up on the local tasks issue?
3) We're removing the form building part of this function. This is also used by a couple of other page callbacks and will be removed in #1974408: Convert aggregator_page_source() to a Controller (currently blocked by this issue.
4) valdiateForm() is the only boiler plate code that can be removed by using FormBase, so I've refactored this to use it.
Comment #46
tim.plunkettFormBase also means t() can become $this->t()
Comment #47
kim.pepperCoverts t() -> $this->t()
Comment #48
kim.pepperThis addresses timplunkett's concerns in #41. We now use buildForm() in the base form controller.
Comment #49
tim.plunkettThis looks great, thanks for bearing with me @kim.pepper!
Comment #50
webchickHm. A bit concerned about this. The generic "SomethingSomethigBase" name makes it sound like it's a class intended to be extended by other modules, but it's extremely Aggregator module specific. Can we throw "Aggregator" in the class name? (Yes, I realize it's namespaced to aggregator module, but I'm not sure people will get that when it comes up as a class they see in their IDE.)
Comment #51
kim.pepperHow about
AggregatorCategorizeItemForm
? Dunno.Comment #52
kim.pepperRenamed to
AggregatorCategorizeFormBase
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentedComment #53.0
ParisLiakos CreditAttribution: ParisLiakos commentedAdded follow up
Comment #54
webchickCommitted and pushed to 8.x. Thanks!
Comment #55
HyperGlide CreditAttribution: HyperGlide commented@afeijo++
Comment #56.0
(not verified) CreditAttribution: commentedUpdated summary