Config translation code documentation and small code clean up [#2137223]

Updated: Comment #N

Problem/Motivation

tracking these so I dont forget them.

also, want to see what testbot says about the changes to make sure the unused uses are actually unused and the type hinting stuff is ok.

Proposed resolution

not alone enough to cause a reroll of the add to core patch, I think.
hold off until it's in core, or another issue causes it to need the core patch to be redone, or.. something here is found to be bad enough to be a blocker on getting into core.

changes:

removes some contractions (under assumption makes it easier for non-native english speakers to understand without them)
turns eg. into for example
makes the @file class name match the actual class name/filename
removed unused uses
makes types match in @param

Remaining tasks

read all the code
add jhodgdon's notes (not blocking either)

User interface changes

API changes

Comment	File	Size	Author
#7	2137223-7.patch	14.91 KB	tstoeckler
#7
#2	2137223-config_translation-cleanup-2.patch	14.68 KB	YesCT
#2
#2	interdiff-2137223-1-2.txt	7.02 KB	YesCT
#1	2137223-config_translation-cleanup-1.patch	8.06 KB	YesCT
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

YesCT CreditAttribution: YesCT commented 16 November 2013 at 13:09

Status:

Active

» Needs review

File	Size
2137223-config_translation-cleanup-1.patch	8.06 KB

needs review for testbot, not done reading yet

Comment #2

YesCT CreditAttribution: YesCT commented 16 November 2013 at 15:10

File	Size
interdiff-2137223-1-2.txt	7.02 KB
2137223-config_translation-cleanup-2.patch	14.68 KB

some more little things, like (optional) in @param.
still not done reading. (and I think still nothing blocking)

Comment #3

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 17 November 2013 at 02:59

Awesome, looks great. Agreed on not re-rolling the core patch for this. I suppose most of the wrong "Contains ..." etc. are my fault, so sorry, and thanks a lot for cleaning that up!

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 November 2013 at 06:06

Does this include Tim's changes.txt? We need that in our repo first, if we need to roll more patches against it.

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 18 November 2013 at 09:13

Title:	docs and small code clean up	» Config translation code documentation and small code clean up
Project:	(Obsolete) configuration translation for Drupal 8	» Drupal core
Version:	8.x-1.x-dev	» 8.x-dev
Component:	Code	» config_translation.module
Status:	Needs review	» Needs work
Issue tags:		+D8MI, +language-content

Needs reroll against core now.

Comment #6

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 19 November 2013 at 12:52

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -131,11 +130,12 @@ abstract class ConfigTranslationFormBase extends FormBase implements BaseFormIdI
-   *   Page request object.
+   *   (optional) Page request object.
...
-   *   The plugin ID of the mapper.
+   *   (optional) The plugin ID of the mapper.
...
-   *   The language code of the language the form is adding or editing.
+   *   (optional) The language code of the language the form is adding or
+   *   editing.

These are sort of questionable, although I guess I sort of agree. The problem is the following: They are not actually optional by the nature of the class, they *should* really be required. But we cannot add required parameters to buildForm() without breaking FormInterface, that's why they're optional. So I guess I tentatively agree.

Comment #7

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 19 November 2013 at 12:54

Status:

Needs work

» Reviewed & tested by the community

File	Size
2137223-7.patch	14.91 KB

3 files were hidden/shown/deleted

File	Size
2137223-config_translation-cleanup-1.patch	8.06 KB

interdiff-2137223-1-2.txt	7.02 KB
2137223-config_translation-cleanup-2.patch	14.68 KB

Here's a re-roll.

No actual changes so going straight to RTBC. There's no reason to credit me on this one.

Comment #8

YesCT CreditAttribution: YesCT commented 20 November 2013 at 15:57

Regarding #6, I think that is an excellent reason to add an explanation there about why they are not really optional.

I'm ok with getting this much in, and doing the rest later. (I'm taking my broken computer in to get it fixed... hopefully I can bring it back home with me fixed today. But if not I'm going to be slower than usual to get back to this, as I'll have to get a different computer set up first.)