Fix Coding Standards

Great! I'm a bit hesitant about such CSS changes but that files have gone now anyway.

Note that this introduced some fatal errors, fixed in #3135628: Declaration of EntityInlineForm::getTableFields($bundles) must be compatible with InlineFormInterface::getTableFields(array $bundles).

Thanks for this quick feedback. My bad. Reverting this for now to be safe and sure.

Great. I think there is a command for generating that patch automatically.
Also I have seen that like me you are a big fan of type hints. as far as I know, in our interner test classes we can add them without Problems, but everywhere I having a type hint is a signature change and brakes compatibility and is therefore forbidden.

Hi,

Creating a patch for this issue.

Please review the patch.

Thanks.

I will review this last patch.

I found some phcs erros and I fixed them.

Awaiting for review.

I triggered a test of #26 (you can / should do that yourself when submitting a patch, if the test does not trigger automatically) and as you can see the patch failed PHPLint. Setting the status to "Needs work".

-    '#submit' => [['\Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormComplex', 'closeForm']],
+    '#submit' => [
+      [
+        '\Drupal\inline_entity_form\Plugin\Field\FieldWidget\
+        InlineEntityFormComplex', 'closeForm',
+      ],
+    ],

This is just wrong. Coding standards are not telling you to do that.

@TR my phpcs show me a error in this code that you commented.

FILE: /home/guilhermerabelo/Contribution/Project2/recommended-project/web/modules/contrib/inline_entity_form/inline_entity_form.module
---------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------
 123 | ERROR | The array declaration extends to column 111 (the limit is 80). The array content should be split up over multiple lines
 123 | ERROR | The array declaration extends to column 110 (the limit is 80). The array content should be split up over multiple lines
---------------------------------------------------------------------------------------------------------------------------------------

So i decide to fixe this, but if you prefere I leave it the way it was, let me know.

This new patch fixed the erros in tests.

It's HOW you changed that line that's the problem. This is not a valid class name:

'\Drupal\inline_entity_form\Plugin\Field\FieldWidget\
        InlineEntityFormComplex'

You can't just add new lines and spaces in the middle of a string that represents a class name.

All rigth @TR, now i understand and make the changes that you mentioned. I add the new patch.

Thanks, waiting for review.

I will try to review it.

First, a reroll of InlineEntityFormSimple.php 144 was done, then a correction in the array of line 137/144, a warning was found in 598 of InlineEntityFormComplex.php that when fixing it generates a phpcs error, therefore better to ignore.

#32 is missing an interdiff so it's hard to tell what has been changed.

HOWEVER, #32 does introduce at least one syntax error, as shown in the the test results, AND it still hasn't addressed the problems I raised in #27 and #29.

Ok so,

I worked on #30 to solve the problem with the "invalid" class name generated after breaking the array.

Also worked on another array which needed to be split and on the Lint error (which was only a colon being used with a null coalescing operator).

Interdiff/sniffer are attached with the patch. Hope it solves the problem.

Kindly review it :)

I'll review this last patch.

I reviewed the patch #34 and the Coding Standards errors were all fixed. The tests have 4 errors with or without the patch, so I assumed that is not the patch that is causing the tests failures. I'll mark the status as RTBC because the subject of this issue was to fix the Coding Standards.

Thanks for the work here and the reasoning for RTBC. It lloks wrong to me though.

> The tests have 4 errors with or without the patch,

Looking on https://www.drupal.org/node/1181848/qa, i can't reproduce this.
D10 is red, and Mysql-pre-9.7, all others are green.
Please use green tests to compare.

@geek-merlin, looks like the erros in the tests appeared after the patch in #26.

I didn't have the time to compare it with #22 but i will try to do it today. Also, do you prefer me to send a patch or to work on the open MR for this issue?

I talked to Bruno, and as he is out of time i'll be working on this one.

I resolved the local tests problem and started to fix the coding standards errors and warnings.
I couldn't solve it all but got a pretty good result. I also couldn't make an interdiff file, because when I tried to apply the #34 patch I got some errors and i'm sorry about that.

Changing status to "Needs review" and waiting for the feedbacks =)

hi, i'll work on it :)

I reviewed the patch from comment #42 and the errors were fixed and all the tests are passing. There are still some warnings left on PHPCS (attached .txt), so if you find it necessary, we can work on them. Other than that, I'm changing the status to RTBC since the main issue was fixed :)

> There are still some warnings left on PHPCS (attached .txt)

This issue is about fixing them, so NW.

Updated patch #42 by addressing #45 and some more PHPCS issues, please review it.

I'll work on this again =)

I fixed the patch #46 by removing the dependency injection attempt. The tests are not failing anymore.

The PHPCS only has 2 remaining warnings. I'll leave this to someone who can fix both of them. =)

-    '#submit' => [['\Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormComplex', 'closeForm']],
+    '#submit' => [
+      [
+        '\Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormComplex',
+        'closeForm',
+      ],
+    ],

Why not just
'#submit' => [[InlineEntityFormComplex::class, 'closeForm']],
along with the appropriate use statement? This construct appears 5 times in the patch, and the ::class form can be used in all five places. This would fix the coding standards error as well as make the code easier to read.

I fixed the patch #46 by removing the dependency injection attempt. The tests are not failing anymore.

That is unnecessary - the dependency injection could be left in, just complete the attempt. In patch #46 the content_translation.manager service is injected into InlineEntityFormBase, but that patch fails to modify the subclass InlineEntityFormComplex to account for the change in its parent::__construct(). That fixes one of the test failures. The other test failure is because SimpleWidgetTest tries to use content translation but doesn't declare content_translation as one of its dependencies. That's easily remedied by adding content_translation to the $modules variable as is done in other tests.

But that does bring up the question of whether inline_entity_form should depend on the core content_translation module, and if not then how should the optional dependency be handled throughout IEF. If IEF should not be dependent on content_translation then it seems like it might make more sense to put the content_translation integration into TranslationHelper rather than try to do it in the field widget by injecting a dependency that might not exist.

Regardless, I disagree with @geek-merlin from #45. I think trying to eliminate ALL coding standards issues here makes this far more difficult to get committed. 99% of the patch is correct already. The one or two remaining things, like the question of dependency on content_translation, can be safely left to other issues IMO.

Thanks @TR for that thorough review and anti-chaos work, that's really appreciated.

And please everyone, before adding chaos, ask here.

> Regardless, I disagree with @geek-merlin from #45. I think trying to eliminate ALL coding standards issues here makes this far more difficult to get committed. 99% of the patch is correct already. The one or two remaining things, like the question of dependency on content_translation, can be safely left to other issues IMO.

Yes, i assumed that we're doing the chores here. If stuff is more complex and needs discussion or refactoring, please add a @todo and good it is.

As of the translation_manager, i suppose it's as simple as using the second argument of Container::get() (but i might be wrong).

Hi I revised and applied the patch #46, I ran this command phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml /path/to/drupal/example_module

After that I didn't see phpcs errors and then I'm moving for RTBC.

@elber: Please, before commenting, read what was written and do NOT RTBC a patch that does not have GREEN tests. Thank you.

I'll try to do the remaining adjustments :)

I fixed one more phpcs error and added the @todo comment about the DI that needs to be done. Hope someone reviews it =)

hi, i'll review patch #56

Hello!
I applied the patch #56 and fixed some remaining PHPCS issues.

The things that are left to do know are DI and 'only string literals should be passed to t() where possible.'

Regarding the issue: 'Doc comment short description must be on a single line, further text should be a separate paragraph', I feel like it's only a warning and there's no need to fix it.

The final warning is 'Open page callback found, please add a comment before the line why there is no access restriction', it's possible to fix by only adding a comment, however, I'm sure why there is no access restriction.

I talked to @tauanygb to understand the patch #59 and we get to the conclusion that the patch #56 was wrongly generated by me, because it only contains changes to the patch #49 instead of all changes.

After analysing and testing, I generated this new patch (#60) that contains all PHPCS errors fixed, except for the DI warning. All tests are passing with this patch.

Sorry for the little mess. I hope someone reviews it =)

Hello, I will review this last patch =).

Hello, applied the #60 patch (3135178-60.patch), reviewed it.

This is the only warning showing after running:

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,js .

FILE: /modules/contrib/inline_entity_form/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
-----------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------
472 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------------------------------------------------------------

Confirming @alanmoreira on #60. All other errors are fixed, except the DI warning, and tests are all green.

As mentioned by @geek-merlin to only RTBC if all tests are green, so, maybe RTBC?

Cheers.

Hello!!
All the PHPCS issues were fixed and all the tests are passing, so I'm changing the status to RTBC. If it's necessary to fix the DI problems, we can open a new issue to work on it :)

Committed, thanks!