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 |
---|---|---|---|
#30 | 1987818-convert-date-format-language-30.patch | 4.22 KB | vijaycs85 |
#30 | 1987818-diff-26-30.txt | 1.93 KB | vijaycs85 |
#31 | Localize date formats.png | 76.56 KB | vijaycs85 |
#31 | Edit date format | drupal.dev_.png | 41.07 KB | vijaycs85 |
#26 | drupal-convert_system_date_format_overview_page-1987818-26.patch | 4.17 KB | jlbellido |
Comments
Comment #1
MattDanger CreditAttribution: MattDanger commentedComment #2
MattDanger CreditAttribution: MattDanger commentedComment #4
MattDanger CreditAttribution: MattDanger commented#2: convert-system_date_format_language_overview_page-1987818-2.patch queued for re-testing.
Comment #5
pdrake CreditAttribution: pdrake commentedI believe this should be in a Controller directory. Otherwise, this looks pretty good to me.
Comment #6
MattDanger CreditAttribution: MattDanger commentedMoved LanguageController.php to lib/Drupal/system/Controller
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedShould be
Contains \Drupal\system\Controller\LanguageController
we remove those from the controllers
misses @return docs
unneeded whitespace
this should return a render array
Comment #8
disasm CreditAttribution: disasm commentedattached patch fixes comments in #7.
Comment #9
disasm CreditAttribution: disasm commentedComment #11
disasm CreditAttribution: disasm commentedSyntax for render array was wrong. Looked it up, and should be good now. Ran failed test locally, and passed without issues.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedyeah its perfect now, thanks!
Comment #13
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #14
alexpottNeeds a reroll
Comment #15
aks22 CreditAttribution: aks22 commented#11: drupal-convert_system_date_format_overview_page-1987818-11.patch queued for re-testing.
Comment #17
jlbellidoRerolled #11 and removed tag
Comment #18
penyaskitoComment #19
jlbellidoI have reviewed my reroll #17 and it was wrong:
- It add code that in HEAD is deleted.
- It doesn't delete 'system_date_format_language_overview_page' function from system.admin.inc. This function is deleted in #11.
For fix this , i add a new reroll.
Comment #20
YesCT CreditAttribution: YesCT commentedsending to the testbot by changing status to needs review.
Comment #21
Gaelan CreditAttribution: Gaelan commentedComment #22
vijaycs85Tested patch in #19 and working as expected
Steps below:
1. Applied patch
2. Enabled language module
3. Added new language
4. Visited admin/config/regional/date-time/locale (I couldn't find a link in UI to go this path though)
Attaching screenshot of the page.
RTBC from my side.
Comment #23
Gábor HojtsyFeel free to continue working on this, as per Dries' post this is still possible: http://buytaert.net/drupal-8-api-freeze (see Deprecating Drupal 7 APIs which according to @xjm applies to issues such as this one).
Fix tagging for D8MI.
This is just for date formats. How does this become a "LanguageController"? Sounds very generic.
May be too much to change paths, but 'locale' is so-so-so wrong!
Also should use string concatenation, not double quotes.
Comment #24
Gábor HojtsyComment #25
Gábor HojtsyAdd API change tags.
Comment #26
jlbellidoAdded the following updates:
- Renamed "LanguageController" class to "LanguageDateFormatController".
- Changed double quotes to string concatenated.
- Changed paths "admin/config/regional/date-time/locale/$langcode/reset" to "admin/config/regional/date-time/$langcode/reset"
This last change needs work because i think that parent path "admin/config/regional/date-time/locale" should be changed to "admin/config/regional/date-time/" too. This should do changes in Tests , ect.. Should it?
Comment #27
Gábor HojtsyIs this page still doing something useful? Languages are already assigned on the main editing page. What do the edit controllers do? Changing the path here is certainly not good in itself so long as the edit paths are nt reacting to it. I think we should figure out first and foremost if this is not just leftover code that should just be removed.
Comment #28
YesCT CreditAttribution: YesCT commentedneeds review just to let the bot have a go at testing it.
As #27 we still need to look and see if we need this at all.
Comment #30
vijaycs85Re-rolling...
Reg #27, we have two pages related to language for dateFormat(see attached #31). Don't see any link for this locale page though.
Comment #31
vijaycs85Attaching screenshots.
Comment #32
Gábor HojtsyWhat do these pages even do and how do they relate? #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed claims the combination of UIs is broken and regressed features from Drupal 7 unintentionally.
Comment #33
jibranIt is a simple conversion.
Comment #34
Gábor Hojtsy#30: 1987818-convert-date-format-language-30.patch queued for re-testing.
Comment #35
Gábor HojtsyYeah, I agree it is best to just get this in for now, but the whole date localization UI / process / backend is a disaster in Drupal 8: #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed
Comment #36
webchickLooks good.
Committed and pushed to 8.x. Thanks!
Comment #37
Gábor HojtsyAll right, now let's move onto the heap of issues in #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed and fix the bugs.