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

Files: 
CommentFileSizeAuthor
#7 2137223-7.patch14.91 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 59,003 pass(es). View

Comments

YesCT’s picture

Status: Active » Needs review
FileSize
8.06 KB
PASSED: [[SimpleTest]]: [MySQL] 818 pass(es). View

needs review for testbot, not done reading yet

YesCT’s picture

FileSize
7.02 KB
14.68 KB
PASSED: [[SimpleTest]]: [MySQL] 818 pass(es). View

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

tstoeckler’s picture

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!

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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.

tstoeckler’s picture

+++ 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.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.91 KB
PASSED: [[SimpleTest]]: [MySQL] 59,003 pass(es). View

Here's a re-roll.

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

YesCT’s picture

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.)

tstoeckler’s picture

Yeah, I'm also fine with it for now.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice clean-up, thanks!

Committed and pushed to 8.x.

Gábor Hojtsy’s picture

Fix tagging.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.