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

I'm working on this for the DrupalCon Baltimore mentored core sprints.

I've completed an initial pass at adding @internal to all core controllers. I went through all *.routing.yml files and used _controller: [...] lines as a reference to find all core controller class files, and added @internal to the comment block to all the controller class files I found.

Rebased the patch on latest commit, looks like there was a different commit that got included in the patch.

Correcting a backwards patch.

Attempt # 4.

Me and my collegues are working on this at the Baltimore Code Sprint right now...

Any clues why the test fails? I've run the System.Drupal\system\Tests\System\ErrorHandlerTest locally with and without the patch. The test runs fine without the patch applied. I'll continue investigating with leanderl and our other collegues.

The patch only changes comments, in theory it shouldn't be causing any tests to fail.

I did run into an issue with my first patch attempt where there had been a completely unrelated change that was included, but I was able to rebase and exclude that change.

Actually, the test seems to create a PHP tests and checks for the error on row-number in the file ErrorHandlerTest.php.

I, Marcus, Line and Leanderl are sitting here in Baltimore and checking for this, will try to apply a fixed patch soon.

Previous patch failed because tests referred to specific line numbers, we adjusted this to make the test pass.

leanderl, marcusml, Andreas Hansson and I worked with the patch together.

Updating issue credit based on #17. Thanks @linef!

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

Re rolled.

Either pushed it up early or we have to again re roll it after some time again and again.

Thanks @pk188 for the reroll.

Can't we declare controllers as a not supported API, instead? It feels really a bit stupid to add @internal to all those things.

I am working on this issue at DrupalCon Vienna.

I am working on this issue with @Valthebald and @chenderson at DrupalCon Vienna.

Thanks

This is not a novice issue as per comment Comment #24

Removing Needs subsystem maintainer review tag. since this effects nearly every system and module I don't think that applies.

Regarding #24

Controllers are already declared @internal in our docs https://www.drupal.org/node/2562903/ but also I think in the proposed changes #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation

I think #2550249 would be the place to argue that we should not explicitly add the @internal tags to controllers(or other things that our docs also say are internal).

Or if we don't think we should move forward with these types of changes we should address that in the meta issue #2873395: [meta] Add @internal to core classes, methods, properties

Re-rolled.

The patch no longer applies so this needs another re-roll and a review to get this in.

Re-rolled the patch.

Still need reroll

Here is the rerolled patch. I also change 3 @internal mention with a breaking-line before for consistency with every others @internal annotation, see interdiff-2873668-35-38.txt.

I'm at DrupalEurop and reviewing the work.

@wengerk, Thansk for the reroll. Next time, please provide two consecutive patches: first a reroll, then the additional changes. Reroll requires a different kind of review than a change. But very good you provided a diff for the changes, that helps very much.

#38 patch applies cleanly. Compared #35 and #38 patches and found no other changes than expected.

After applying the patch, I find various controllers that have no @internal. (I've searched for extends Controller, Controller {). I used the following criteria:
- Controllers that are bound to individual routes
- All controllers in tests

All unmarked route controllers are either related to external URLs or can be considered part of the API (e.g. Entity list controllers).

Todo: Various controllers in tests are not marked @internal, other test controller are. A.o. search for 'TestController'.

Thanks for your review !
Here the changes from the suggestions in #41

I am wondering whether we could add a codestyle rules to automatically check for that.

I changed the proposed solution to match with the described task and the given patches.

Hi, on drupal-8.8.x-dev patch fail to apply on :

• - core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
• - core/modules/system/src/Tests/System/ErrorHandlerTest.php
• - core/modules/system/tests/modules/conneg_test/src/Controller/TestController.php

#42 is not getting applied. I would like to work for #41

I have added @internal tag to the controller classes for version 8.9.x. Please review.

I have created a patch by adding @internal to all the controller classes , methods etc in drupal core with version 8.9.x. Please review.

I have got phplint failed error for #55. Can anyone suggest me how to correct this error??

I have re-created a patch for controller doocumentation.
Please review.

I have tried recreating a patch. Hope this will pass the test.

There is a syntax error in the patch.

PHP Parse error: syntax error, unexpected 'FormController' (T_STRING), expecting '{' in /var/www/html/core/lib/Drupal/Core/Entity/HtmlEntityFormController.php

I have tried correcting the php parse error.

Fixed the failing test. Here is the updated patch.
Kindly review

Hii @Dinesh18 #63 not getting applied coming whitespace errors.

Adding Amsterdam2019 tag, for work in DrupalCon Amsterdam 2019.
Needs review and testing of patch and fix of the issue in the comment #67.

I am going to check this out now at DrupalConAmsterdam2019

I'm going to work together with Frederikvho

Removed all whitespaces. Please review.

I tested this on 8.9.x and it works. The patch error has been resolved in comment #71.

May we just wait for the tests to complete?

Fixed coding standards issues, but not sure on how to fix this:

 Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::testUncaughtFatalError
Failed asserting that 200 is identical to 500.

/var/www/html/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:364
/var/www/html/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:113

I tried to fix the test fail error. Not sure if it will work or not.

The failing test is the following one.

  /**
   * Tests displaying an uncaught fatal error.
   */
  public function testUncaughtFatalError() {
    $fatal_error = [
      '%type' => 'TypeError',
      '@message' => 'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 62',
      '%function' => 'Drupal\error_test\Controller\ErrorTestController->Drupal\error_test\Controller\{closure}()',
    ];
    $this->drupalGet('error-test/generate-fatals');
    $this->assertResponse(500, 'Received expected HTTP status code.');
    $message = new FormattableMarkup('%type: @message in %function (line ', $fatal_error);
    $this->assertRaw((string) $message);
    $this->assertRaw('<pre class="backtrace">');
    // Ensure we are escaping but not double escaping.
    $this->assertRaw('&#039;');
    $this->assertNoRaw('&amp;#039;');
  }

The controller method that should cause a fatal error is ErrorTestController::generateFatals().

public function generateFatals() {
  $function = function (array $test) {
  };
  $function("test-string");
  return [];
}

The patch is correcting the argument passed to that anonymous function, which is the reason why the fatal error would be thrown. The wrong parameter is intentional, and it should not be "corrected."

 /**
  * Controller routines for error_test routes.
+ *
+ * @internal
  */
 class ErrorTestController extends ControllerBase {
 
@@ -59,7 +61,7 @@ public function generateFatals() {
     $function = function (array $test) {
     };
 
-    $function("test-string");
+    $function(["test-string"]);
     return [];
   }

(The third time should go fine.)

As reminder, whenever the ErrorTestController.php file is edited, the following lines could need to be edited too.

    $fatal_error = [
      '%type' => 'TypeError',
      '@message' => 'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 62',
      '%function' => 'Drupal\error_test\Controller\ErrorTestController->Drupal\error_test\Controller\{closure}()',
    ];

62 in on line 62 must be corrected to the number of the line containing $function("test-string"); which, with the changes made here, became line 64.

#85 looks good.. Cleany applied.
Thanks @kiamlaluno

Needs a 9.0.x patch.

git apply -3 drupal-set-controllers-as-internal-2873668-85.patch
error: patch failed: core/lib/Drupal/Core/Entity/Controller/EntityViewController.php:13
Falling back to three-way merge...
Applied patch to 'core/lib/Drupal/Core/Entity/Controller/EntityViewController.php' cleanly.
error: core/modules/tracker/src/Controller/TrackerPage.php: does not exist in index
error: core/modules/tracker/src/Controller/TrackerUserRecent.php: does not exist in index
error: core/modules/tracker/src/Controller/TrackerUserTab.php: does not exist in index

TrackerPage, TrackerUserRecent, and TrackerUserTab were removed in #3097454: Remove tracker.module BC layers.

I removed these from the patch and re-applied to 9.0.x successfully wit a 3-way merge.

I've re-uploaded the patch in 85 as 8.9.x-88.patch and then uploaded the 9.0.x-88.patch for 9.0.x. An interdiff between these two patches is confusing because the interdiff suggests the pages exist in the second patch. They do not. Grepping for TrackerPage, TrackerUserRecent, and TrackerUserTab in the 9.0.x patch shows that those are no longer there but do exist in the 8.9.x patch.

Created for Drupal 9.5.x dev version please test and review this.

For drupal 9.5.x dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-9.5.x-dev-95.patch

Please check and review

Patch doesn't apply, looks like there's a bit of local work on /vendor/coder included?

For drupal 9.5.x dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-9.5.x-dev-97.patch.patch

For Drupal 9.5.x. dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-9.5.x-dev-98.patch

For drupal 9.5.x dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-9.5.x-dev-99.patch

For Drupal 9.5.x dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-9.5.x-dev-100.patch

For Drupal 9.5.x dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-101.patch

-        'Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}(): Argument #1 ($test) must be of type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 65' :
-        'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 65',
+        'Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}(): Argument #1 ($test) must be of type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 64' :
+        'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 64',
       '%function' => 'Drupal\error_test\Controller\ErrorTestController->Drupal\error_test\Controller\{closure}()',
     ];
     $this->drupalGet('error-test/generate-fatals');

It seems the line number is wrong, since that exception message isn't found in the page output.

https://www.drupal.org/files/issues/2022-07-11/drupal-set-controllers-as-internal-2873668-102.patch

For drupal 9.5.x dev

Thank you #102
Can now someone review this patch
https://www.drupal.org/files/issues/2022-07-11/drupal-set-controllers-as-internal-2873668-109.patch

I will review this patch

All the @insert are placed properly. The patch is working fine and updating to RTBC.

The patch applies successfully and works fine. Dont know why it was moved to Needs review again!

Great work on this so far!

One thing we should add is that we should typically explain why something is marked as internal. For example, for plugins we have:

 * @internal                                                                    
 *   Plugin classes are internal. 

So, we should do something similar for this issue, like:

 * @internal                                                                    
 *   Form and route controllers are internal. 

@Anjali Rathod and @JatinGupta40, Thank you for reviewing this issue!

The automated testing infrastructure tells us whether the change set still applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

In this case, it would be good to post specifics of how you checked that all the @internal were properly placed, and how you ensured that no controllers were missed. Describe your review process in detail. For example, post exact commands you used, such as specific grep commands, inside <code> tags (not as screenshots) and what the results of those commands were.

Also see the issue credit guidelines for more information on which kinds of contributions are credited.

And @aarti zikre, in the future, when using a patch workflow, provide interdiffs for your patches. That allows reviewers to evaluate your changes. Thanks!

This is a good candidate to commit during the beta.

We also need versions of the patch that work with 10.1.x and 10.0.x. Mostly this is just a simple matter of removing hunks that are for modules that were removed in Drupal 10:

error: core/modules/aggregator/src/Controller/AggregatorController.php: does not exist in index
error: core/modules/aggregator/tests/modules/aggregator_test/src/Controller/AggregatorTestRssController.php: does not exist in index
error: core/modules/ckeditor/tests/modules/src/CkeditorOffCanvasTestController.php: does not exist in index
error: core/modules/image/src/Controller/QuickEditImageController.php: does not exist in index
error: core/modules/quickedit/src/QuickEditController.php: does not exist in index
error: patch failed: core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:95
error: core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php: patch does not apply

Only that last line for UncaughtExceptionTest needs to be fixed (again).

This issue is tagged for first time contributors at DrupalCon Prague 2022.

Unfortunately this does not meet the criteria for a novice issue in its current state when it is so many versions behind.

These issues were novice at one point, but the way they need to be solved is one at a time, during the beta phase for a release, and generated using scripts and static tooling.

Hiding the old patch files for clarity.