Add @internal to core form classes, methods, and properties

Hello! I'm at #drupalcon baltimore and I'm working on this issue :). I've talked to @naveenvalecha and @eojthebrave about how important this issue is and how it's a very high priority. So far I have a working copy that I've made a short while ago https://github.com/nathanv/core. I'll be doing my work there in a branch https://github.com/nathanv/core/tree/edit_add_internal_to-2873696 . Feel free to come help me there. I'm hanging out in 301 in the Conference Centre and also in #drupal-docs on irc.

Hello! I have here a WIP. It's probably almost done. I did a tonne of work somewhat quickly so I might have missed some style stuff. I'm uploading it now but will be checking it over right away.

My method was to do the following:

• git clone core to a clean folder, load up PhpStorm and launch the New Project From Existing Files Wizard.
• Add core via this with the "Source files are in a local directory, no Web server is yet configured." option selected.
• I then used PhpStorm's nifty subclass hinting feature. I went to lib/Drupal/Core/Form/FormBase.php and hovered my mouse over the editor gutter which displays the indicator that the class has subclasses.
• I then clicked on it and went to all the subclasses and implemented the @internal piece. When there was no docblock, I created one. I hope they are all ok. If not, I am happy to reroll as necessary.

Sorry folks... wrong process here for creating patches... I pulled down just core instead of all of Drupal. This should work.

Thanks @nvexler! Excellent work documenting your specific process for further contributors.

This no longer applies unfortunately. Which makes reviewing this quite hard. This is looking wonderful already though!

BundleModerationConfigurationForm has been removed since the patch in #7 was created (see #2843083: Select entity type / bundle in workflow settings).

Re-rolled.

I'will help with this one during DrupalCon Vienna

Working on it during the DrupalConEU Vienna.

Tagging

Are *FormBase classes internal? I thought that all/most base classes were a part of the API.

The following abstract base classes are marked as internal in the patch above.

• * core/lib/Drupal/Core/Entity/BundleEntityFormBase
• * core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase
• * core/modules/locale/src/Form/TranslateFormBase
• * core/lib/Drupal/Core/Entity/EntityConfirmFormBase
• * core/lib/Drupal/Core/Entity/EntityForm
• * core/lib/Drupal/Core/Form/ConfigFormBase
• * core/lib/Drupal/Core/Form/ConfirmFormBase
• * core/modules/action/src/ActionFormBase
• * core/modules/config_translation/src/Form/ConfigTranslationFormBase
• * core/modules/field_ui/src/Form/EntityDisplayFormBase
• * core/modules/field_ui/src/Form/EntityDisplayModeFormBase
• * core/modules/filter/src/FilterFormatFormBase
• * core/modules/image/src/Form/ImageEffectFormBase
• * core/modules/image/src/Form/ImageStyleFormBase
• * core/modules/language/src/Form/LanguageFormBase
• * core/modules/locale/src/Form/TranslateFormBase
• * core/modules/path/src/Form/PathFormBase
• * core/modules/search/src/Form/SearchPageFormBase
• * core/modules/system/src/Form/DateFormatFormBase
• * core/modules/views_ui/src/Form/Ajax/ViewsFormBase
• * core/modules/views_ui/src/ViewFormBase

Additionally the following abstract classes might be considered part of the API because they are abstract:

• * core/modules/user/src/AccountForm

The following base form class is in form_test only and I think that is fine to be @internal.

• * core/modules/system/tests/modules/form_test/src/Form/FormTestTableSelectFormBase

Please review and discuss these to make sure that we actually want to make them @internal

The previous patch in #11 not apply on 8.5.x for multiple reasons:

The new patch 2873696-12.patch is based on the commit b4ce4af60c and should works now.

Here why the previous patch didn't applied:

• core/modules/system/src/Tests/Form/StubForm.php has been removed.
• core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php has been removed.
• core/modules/statistics/src/StatisticsSettingsForm.php has changed since the previous patch.
• core/modules/system/src/Form/CronForm.php has changed since the previous patch.

Valid point, base forms should not be marked internal per description in the parent issue

As suggested by @mradcliffe in #16 & discuss with @valthebald & @gido, we remove the @internal tag on the abstract Base classes but keep them on:

• core/modules/system/tests/modules/form_test/src/Form/FormTestTableSelectFormBase

We suggest to also not tag as @internal the following classes:

• core/modules/user/src/AccountForm
• Drupal\Core\Entity\ContentEntityForm
• Drupal\Core\Entity\EntityDeleteForm
• Drupal\Core\Entity\EntityForm

I agree, @wengerk. I looked at Examples module, which suggests extending ContentEntityForm and EntityForm.

The exception might be for EntityDeleteForm. Examples module suggests to use EntityConfirmFormBase (config entity) and ContentEntityConfirmFormBase (content entity).

I updated the patch and made a review. I found and tagged the following new Form classes in 8.5x:

• Drupal\form_test\Form\FormTestRadiosCheckedForm
• Drupal\ajax_forms_test\Form\AjaxFormsTestImageButtonForm
• Drupal\ajax_forms_test\Form\AjaxFormsTestAjaxElementsForm
• Drupal\Tests\system\Functional\Form\StubForm
• Drupal\form_test\Form\FormTestOptionalContainerForm
• Drupal\media\MediaTypeForm
• Drupal\media\MediaForm
• Drupal\media\Form\MediaTypeDeleteConfirmForm
• Drupal\media\Form\MediaDeleteMultipleConfirmForm

removed tag from ContentEntityForm and EntityForm.

If you have a neighbor looking to help, they could add an explanation of form classes as discovered by @wengerk, @gido and @valthebald above that are not marked as @internal to the issue summary under the proposed resolution section.

As suggested by @mradcliffe, I updated the Proposed Resolution section.

Just for documenting the way I reviewed the patch:

1. I Applied the patch on a clean 8.5.x branch
2. I Opened the hierarchy class tree with my IDE (with PHPStorm, ctrl+c) to find all children classes of Drupal\Core\Form\FormBase
3. checked one by one if we missed something (thankfully the IDE highlight changed files to make this easier)

1. Removed "Needs issue summary update" because issue summary is updated in #23
2. Repeated steps outline in #24 and ensured that all child classes (with exceptions) now have @internal tag
3. Moving to RTBC

Committed ee0f806 and pushed to 8.5.x. Thanks!

Great work on this, everyone!

Added credit for valthebald for mentoring