Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#28 | 1946454-system-admin-confirm-form-28.patch | 22.07 KB | ParisLiakos |
#23 | 1946454-system-admin-confirm-form-23.patch | 22.05 KB | kim.pepper |
#23 | interdiff.txt | 2.53 KB | kim.pepper |
#17 | system-confirm-form-1946454-17.patch | 21.69 KB | tim.plunkett |
#17 | interdiff.txt | 5.61 KB | tim.plunkett |
Comments
Comment #1
kim.pepperComment #2
kim.pepperFirst go at converting the delete date format confirm form to form interface. This patch does not convert everything in the system.admin.inc.
Comment #3
kim.pepperTag was accidentally removed
Comment #4
kim.pepperAdded date_format_localize_reset form.
Comment #5
kim.pepperComment #6
kim.pepperNot sure I know how to convert the remaining confirm_form code in
system_modules
andsystem_modules_uninstall
. Looks like it needs to be refactored to separate some of the business logic from the form handling.Comment #7
andypostLet's see what bot say.
This forms used as constructors so implementation have empty submitForm()
Also form code a bit refactored to use renderable array against
theme('item_list')
Comment #8
andypostGreen! I think we need a follow-up here to clean-up this forms, seems some elements are not needed anymore
Comment #9
tim.plunkettNo, please NO out of scope changes. This is about converting it from procedural to OO, not changing all of the internals of the form. File a follow-up. Almost all of the interdiff from #7 should be reverted.
All of these docblocks are wrong. Please switch them back, or to {@inheritdoc}
This is bizarre that its only half split across lines.
This should redirect to the path, I believe. We shouldn't be calling it directly here at all.
Comment #10
andypostReverted namespaces in comments
Render preserved as is
Filed follow-up #1964044: Change module list in install/uninstall confirm form to render array
Comment #11
kim.pepperShould be ModulesInstallConfirmForm (not From)
Should be ModulesUninstallConfirmForm
Comment #12
andypostThanx!
The only questionable part a comment - I need help to properly figure out this edge case
Is this comment enough to explain inline usage of public form method?
Comment #13
kim.pepperFixes typos in #11
Comment #14
kim.pepperDoh! Too quick.
Comment #15
andypostRe-roll for #1906474: [policy adopted] Stop documenting methods with "Overrides …"
This out of scope
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedBesides the below, this looks great
lets inject config.factory and use it instead
Comment #17
tim.plunkettOkay, cleaned up a bit.
Comment #18
dawehnerThis is looking really great so far, so here is just a tiny little question which is independent from the actual review.
In general I'm wondering how we could/should document this additional parameters on this kind of methods.
Comment #19
kim.pepperThe phpdoc manual suggests you can put extra @params after the {@inheritdoc} and they will be kept. http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...
Comment #20
dawehnerIt's great that you actually linked the reference!
Comment #21
alexpottNeed extra params documenting... as per #19
Comment #22
andypostRelated issue #1852454: Restrict module and theme name lengths to 50 characters should make changes in module list so I filed #1990544: Convert system_modules() to a Controller
Probably better to convert system_modules() to controller first
Comment #23
kim.pepperRe-rolled against head and updated docs as in #22.
Comment #24
andypostgreat!
Comment #25
PanchoI'm wondering whether something like a naming scheme exists for the route names.
It seems like the router names will be totally inconsistent, if we don't settle with one standard.
In, system.routing.yml, there are already the router names "system.cron" and "system.machine_name_transliterate".
Now you're using "date_format_delete" and "date_format_localize_reset" here.
And in #1990544: Convert system_modules() to a Controller, I'm using "system_modules_list" and "system_modules_list_confirm".
Either of these schemes should be fine, but probably not a mixture of three different schemes.
Also, I'm not sure it makes sense to pull apart system_modules() and its confirm form in two issues.
After either of the patches lands, we need to significantly reroll the other. We can do that though.
[edit:] same for #1993202: Convert system_modules_uninstall() to a Controller
Comment #26
catch@pancho could you open a separate issue for the naming scheme? I think we should sort that out because they're obviously inconsistent now, but it's going to need a dedicated discussion for it.
Comment #27
aspilicious CreditAttribution: aspilicious commentedI think we need a newline before this @param statement
Same
Same =)
Shouldn't stop a rtbc but maybe somebody is bored and wants to reroll :p
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedDisclaimer: i was not bored:)
it just needed a reroll, so i fixed #27
Comment #29
Pancho#26:
You're right and I'm gonna do that.
Actually the route_names are not that important. We can easily streamline them at a later point.
More important is a consistent naming scheme for the classes (= their filenames). We don't want to move around classes and files later, if avoidable. Didn't find anything relevant about that.
Regarding the route names, it however seems that a naming scheme has become prevalent that includes the module name.
We're having 'system_file_system_settings' or 'system_site_maintenance_mode', so I'd rather use 'system_date_format_delete' and 'system_date_format_localize_reset' here.
Don't let my comments unduly delay this patch though.
Comment #30
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.