Remove duplication from FileTestForm and FileTestSaveUploadFromForm

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

The Drupal Contribution Mentoring team is triaging issues for DrupalCon Barcelona 2024, and we are reserving this issue for Mentored Contribution during the event. After September 27, 2024, this issue returns to being open to all. Thanks!

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 ?

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!

Fixed phpcs and phpstan errors.

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.

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

Looks good now ~~ marking this one RTBC

@akulsaxena, Welcome to Drupal!

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

Thanks!