Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#24 | date-formats-1987820-24.patch | 10.33 KB | stevector |
#24 | interdiff24.txt | 2.49 KB | stevector |
#22 | date-formats-1987820-22.patch | 10.56 KB | stevector |
#22 | interdiff.txt | 2.25 KB | stevector |
#20 | drupal-1987820-20.patch | 9.52 KB | dawehner |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedI tried to assign this issue to myself. Going to take a shot at this.
Comment #2
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a first stab at this:
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedComment #4
jibran@file missing
Not needed.
newline missing.
Same as above
Comment #5
dawehnerIn general you should also remove the old code.
ups :)
None of this classes are actually used. Phpstorm shows you the code in grey if it's not used (note: plugin annotation aren't seen).
I'm sorry both these needs some docs.
Wow that's a lot of space
Missing endline at the end of the file.
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedCleaned up the comments as per above.
Comment #7
jibrannewline still missing :)
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedok
Comment #9
cosmicdreams CreditAttribution: cosmicdreams commentedno here
Comment #10
jibranLet's test it.
Comment #11
jibranWe have to remove
system_date_time_formats
in system.admin.inc.Comment #12
jibransystem_date_time_formats
insystem.admin.inc
.Comment #13
jibranFor the test.
Comment #15
dawehnerWhile manually testing the patch I realized that this would break adding a date format and get redirected to admin/config/regional/date-time/formats
which breaks with this change (due to the route system).
We can fix the tests by removing /formats, which is not a problem, as we don't have local tabs here.
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedThis one might fix the two test fails.
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commentedmissing the patch, here it is
Comment #20
dawehnerLet's fix the failures.
Comment #21
stevectorI'm at a table at the Portland sprint right now. We're looking at this patch and scratching our heads over the
use Symfony\Component\DependencyInjection\ContainerInterface;
which is not actually used in the file. We suspect it will be advantageous to actually use ContainerInterface in the future. We'll grab Crell when he gets here.
Comment #22
stevectorWe got some feedback form Crell. Mainly I'm not sure if it makes sense to take out system_date_time_formats() yet as there could be a better answer that replaces that functionality everywhere. I think the that function and related functions need to be a service but it's likely that ideal should stay out of the way of the present conversion issue.
I do think "DateTimeController" is too general. Thoughts?
Comment #23
dawehnerWhat about DateFormatsListing?
I would argue that this should be more like a service, because having a protected method on this object would make it impossible to reuse it. system_get_date_formats seems to be a function you might want to call from outside.
Comment #24
stevectorIs this a decent middle ground short of making date formats be a service themselves? This patch adds injection of the ConfigFactory. This issue is my first route conversion so I'm still feeling out the new guidelines.
Comment #25
stevectorLarry said he'll see this just because it's a conversion issue. Doesn't need "Stalking Crell."
Comment #26
tim.plunkettThis actually duplicates the efforts of #2003892: Convert date formats to config entities
Can we mark this one as a duplicate?
Comment #27
dawehnerThen let's do it.