Remove duplication from FileTestForm and FileTestSaveUploadFromForm

Hi @kim.pepper should I move everything to a trait?

I plan on working on this issue with my team.

-mike

We're working on this in our contrib table.

^^ Not anymore, our bad @ultimike! We thought the 'team' meant the whole mentoring team & newbies. Good luck!

@nico059 and me have created a new file FileTestFormBase.php with the main common code for buildForm function that is extented by the other classes in the other files.

@drubolbol and @nico059 did a great job during the DrupalCon Barcelona mentored core contribution day - making good progress on this issue (and learning a lot about the contribution process along the way.)

Currently, it appears that the changes have caused some tests to fail:

• Drupal\Tests\file\Functional\SaveUploadTest
• Drupal\Tests\file\Functional\SaveUploadFormTest
• Drupal\Tests\file\Functional\RemoteFileSaveUploadTest

See https://git.drupalcode.org/issue/drupal-3368857/-/pipelines/294400 for more details.

-mike

I tried to replace the abstract class by a trait in order to keep original class inheritance/interface implementation intact but the tests results are the same : the "Submit" button cannot be found by #14 stated failing tests.

Dear all,

I try also with Trait, and it work's but .... I think the problem with the 3 failed Test is "somewhere else" in the issue fork code. And NOT in the code done here to resolve this issue ...

What I have done ...
I modify the 3 files in repos/drupal/core/modules/file/tests/file_test/src/Form to use a new Trait:
- FileTestForm.php --> to use the Trait
- FileTestSaveUploadFromForm.php --> to use the Trait
- FileTestFormTrait.php --> the Trait itself
I have created a branch with this code here : https://git.drupalcode.org/issue/drupal-3368857/-/tree/3368857-remove-du...

Tests Works BUT ....
The following tests works:
Drupal\Tests\file\Functional\SaveUploadTest
Drupal\Tests\file\Functional\SaveUploadFormTest
Drupal\Tests\file\Functional\RemoteFileSaveUploadTest
BUT .... only if I test them a more recent issue fork
FYI, as I don't know HOW TO DEV in the latest 11.x Drupal version.... I choose a random other fork more recent to test (I choose 3478771-inputconfigurator-should-expose), and when I run them all works !!

Conclusion
I think something is "wrong" in the current issue fork (3368857) somewhere else
Perhaps is it possible to rebase ALL this issue fork with the latest core version ? And see if the Test works too ?

mattlc → changed the visibility of the branch 3368857-remove-duplication-trait-rebased to hidden.

I have pushed code based on @ultimike and @nico059's approach using an abstract class and the tests appear to pass now. I'm assuming there were some random Functional test failures but they pass once I rerun the pipeline.

There are 3 MRs up but no clear indicator which should be reviewed. Also proposed solution section appears to be remaining tasks.

Hello,

Thank you @anand48, as discussed with both @nico059 and @ultimike at DC Bacelona Trait might be a better solution to keep a simple extension hierarchy.

I worked on the 3368857-remove-duplication-trait.

- minor code style issues fix
- rebased on 11.x
- force pushed to 3368857-remove-duplication-trait

So I think the MR to consider is 9792 from @nico059.

I set the status to Needs review.

MRs should be hidden,

But 9792 has errors.

Just saw this. My bad.
I rework on it.
Sorry for that.

No apology needed!

mattlc → changed the visibility of the branch 3368857-remove-duplication-from to hidden.

mattlc → changed the visibility of the branch 3368857-remove-duplication-from-form to hidden.

Fixed phpcs and phpstan errors.

smustgrave → closed merge request !9644

smustgrave → closed merge request !9975

Issues seems resolved and trait seems good to me.

Closed the other 2 MRs

Thanks everyone for getting this to RTBC. There is just one thing, which I commented on in the MR. I know naming can be a challenge but I do think the method needs a better name.

Hi @quietone

I changed the name of the trait FileTestFormTrait to baseForm as instructed.
Please take a look

Thanks

Minor suggestion regarding the naming convention of trait class ~~ hence moving back to NW.

Hey @ankitv18,

Thanks for pointing out the mistake.
In #33, @quietone mentioned:

I think we need a better name here, one that doesn't have 'trait' in a method name.

I changed the trait name while I should have just changed the method name. I undid the unnecessary changes and changed the method name as suggested.

Thanks
Please review it now.

Looks good now ~~ marking this one RTBC

Rebased and updated fork to latest

The pipeline was run after rebasing and tests are all green - no errors!

@akulsaxena, Welcome to Drupal!

Committed to 11.x and cherry-picked to 11.1.x

Thanks!