Modernize locale.module forms [#2077985]

Part of #2072251: [meta] Modernize forms to use FormBase

Updated: Comment #N

Problem/Motivation

Now that #2059245: Add a FormBase class containing useful methods is in, I was looking at old forms that have empty validateForm() methods, or still use Drupal::service() or t().

Proposed resolution

Convert existing FormInterface forms to extend FormBase

Remaining tasks

User interface changes

API changes

Comment	File	Size	Author
#7	interdiff-2077985-5-7.txt	195 bytes	amitgoyal
#7	drupal8-locale-2077985-7.patch	5.46 KB	amitgoyal
#7
#5	drupal8-locale-2077985-5.patch	5.77 KB	er.pushpinderrana
#5
#2	locale-2077985-2.patch	7.21 KB	tim.plunkett
#2
#2	interdiff.txt	2.9 KB	tim.plunkett
#1	modernize_locale-2077985-1.patch	5.6 KB	Ivan Zugec
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Ivan Zugec CreditAttribution: Ivan Zugec commented 22 September 2013 at 14:48

Status:

Active

» Needs review

File	Size
modernize_locale-2077985-1.patch	5.6 KB

Only LocaleSettingsForm.php had to be updated.

Comment #2

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 2 October 2013 at 22:16

File	Size
interdiff.txt	2.9 KB
locale-2077985-2.patch	7.21 KB

Updated a couple other things

Comment #3

14 July 2014 at 03:09

tim.plunkett queued 2: locale-2077985-2.patch for re-testing.

Comment #4

14 July 2014 at 03:10

Status:

Needs review

» Needs work

The last submitted patch, 2: locale-2077985-2.patch, failed testing.

Comment #5

er.pushpinderrana CreditAttribution: er.pushpinderrana commented 14 July 2014 at 04:11

Issue summary:	View changes
Status:	Needs work	» Needs review

File	Size
drupal8-locale-2077985-5.patch	5.77 KB

3 files were hidden/shown/deleted

File	Size
modernize_locale-2077985-1.patch	5.6 KB

interdiff.txt	2.9 KB
locale-2077985-2.patch	7.21 KB

Unable to apply above patch. Please review updated patch.

Comment #6

dawehner

German

CreditAttribution: dawehner commented 14 July 2014 at 05:24

Status:

Needs review

» Reviewed & tested by the community

The difference in size is replacing configFactory with the config() method which is already in HEAD.

Comment #7

amitgoyal CreditAttribution: amitgoyal commented 14 July 2014 at 10:14

Status:

Reviewed & tested by the community

» Needs review

File	Size
drupal8-locale-2077985-7.patch	5.46 KB

interdiff-2077985-5-7.txt	195 bytes

I believe we don't really need to commit LocaleForm.php in this patch as it simply adds an empty line.

Comment #8

er.pushpinderrana CreditAttribution: er.pushpinderrana commented 14 July 2014 at 10:38

I think @amitgoyal, you took it in wrong way, as per drupal standard there should be a blank line after <?php tag, that was added in previous patch but removed in your patch.

And I think instead of adding new patch instantly, better to suggest through comment and let other people review it.

Thanks!

Comment #9

amitgoyal CreditAttribution: amitgoyal commented 14 July 2014 at 11:15

Thanks for your advice @er.pushpinderrana! I know Drupal standards but I am following the advice from @dcam - https://www.drupal.org/node/1890980#comment-8946211.

This issue is not about fixing Drupal standards and we are committing 1 file just to add 1 extra line. We may fix Drupal standards and I believe there would be more in separate issue.