Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heykarthikwithu created an issue. See original summary.

heykarthikwithu’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 2643752-2.patch, failed testing.

The last submitted patch, 2: 2643752-2.patch, failed testing.

The last submitted patch, 2: 2643752-2.patch, failed testing.

heykarthikwithu’s picture

Status: Needs review » Needs work

The last submitted patch, 6: 2643752-6.patch, failed testing.

The last submitted patch, 6: 2643752-6.patch, failed testing.

The last submitted patch, 6: 2643752-6.patch, failed testing.

heykarthikwithu’s picture

Status: Needs work » Needs review
miro_dietiker’s picture

Status: Needs review » Needs work

Thank you for cleaning up the code.
It's important that with cleanup commits, we are not breaking other pending patches. And as such, if we commit a cleanup, it should cover all similar aspects in one commit.

I see you caught all unused use declaration.
From quickly running code analysis in PHPStorm, i see that there are other deprecated remaining things such as: (still) EntityManager, SelectionBase, entity_view()
Let's clean them up in the same cycle.

+++ b/src/Tests/ParagraphsAdministrationTest.php
@@ -248,9 +249,9 @@ class ParagraphsAdministrationTest extends WebTestBase {
+      'files[field_paragraphs_0_subform_field_image_0]' => FileSystem::realpath('temporary://myImage1.jpg'),

Hm, this method is not declared as static! We should not call it statically!

petermallett’s picture

Title: Remove depreciated methods and unused imports in code base. » Remove deprecated methods and unused imports in code base.
Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.26 KB
14.68 KB

I couldn't get this patch to apply to the current 8.x-1.x -- mostly because a lot of the issues were fixed in other updates already. So I went about making a new version of the patch to remove unused use statements & update the remaining deprecated method calls (also corrected the use of FileSystem::realpath()).

Note: tests are failing but I reviewed them and none of the problems look like they're related to these changes.

miro_dietiker’s picture

Status: Needs review » Postponed

Thank you for the update.
We have a Paragraphs Sprint going on until thursday. Code clean-up commits contain the risk to make other patches not apply anymore. Since there is a high amount of other activity and i don't want to create collateral damage, i will postpone committing code-clean-ups until the sprint ended.

petermallett’s picture

Title: Remove deprecated methods and unused imports in code base. » Remove unused imports in code base.
Issue summary: View changes
Status: Postponed » Needs review
FileSize
12.68 KB

Circling back after a long time to see if we can get this one moving again.
Since it is highly unlikely that a monolithic cleanup patch will ever be applied because it will always conflict with other patches in flight, I propose that we narrow the scope of this issue and break out smaller issues for working on replacing deprecated methods.

Here's a patch to do a smaller cleanup pass: just removes unused imports (and a few minor whitespace tweaks). This is less likely to conflict with ongoing work in other issues.

phpcs used to identify cleanup:
phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md --sniffs=Drupal.Classes.UnusedUseStatement,Squiz.WhiteSpace.SuperfluousWhitespace ./

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Seems like a useful split. testbot also reports 37 fewer coding style issues. I wish they would display an icon directly in the issue, I never look into the detail page.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thank you.

Status: Fixed » Closed (fixed)

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