Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2873395: [meta] Add @internal to core classes, methods, properties
Problem/Motivation
https://www.drupal.org/core/d8-bc-policy#controllers
Core modules contain many route controllers that are bound to individual routes, as well as form classes. These controllers are not part of the API of the module unless specifically marked with an @api tag on the method or class.
Proposed resolution
Add @internal tag to the controller classes
How to fix thisissue
- Read the description for a category in its issue
- Identify and confirm an example. Ask in IRC if unclear.
- Search core for the relevant category.
- Add @internal per the backwards compatibility policy.
- Reviewers should confirm that each @internal mention is appropriate for that category according to the policy.
Remaining tasks
Make the Patch and post it
User interface changes
None.
API changes
There should be no implicit API changes.
Comments
Comment #2
naveenvalechaComment #3
safetypinI'm working on this for the DrupalCon Baltimore mentored core sprints.
Comment #4
safetypinI'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.
Comment #5
safetypinRebased the patch on latest commit, looks like there was a different commit that got included in the patch.
Comment #6
safetypinCorrecting a backwards patch.
Comment #7
safetypinAttempt # 4.
Comment #11
leanderl CreditAttribution: leanderl commentedMe and my collegues are working on this at the Baltimore Code Sprint right now...
Comment #13
marcusml CreditAttribution: marcusml commentedAny 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.
Comment #14
safetypinThe 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.
Comment #15
Andreas Hansson CreditAttribution: Andreas Hansson commentedActually, 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.
Comment #16
linef CreditAttribution: linef commentedPrevious patch failed because tests referred to specific line numbers, we adjusted this to make the test pass.
Comment #17
linef CreditAttribution: linef commentedleanderl, marcusml, Andreas Hansson and I worked with the patch together.
Comment #18
xjmUpdating issue credit based on #17. Thanks @linef!
Comment #19
Wim LeersThis no longer applies unfortunately. Which makes reviewing this quite hard. This is looking wonderful already though!
Comment #20
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled.
Comment #22
pk188 CreditAttribution: pk188 at OpenSense Labs commentedEither pushed it up early or we have to again re roll it after some time again and again.
Comment #23
xjmThanks @pk188 for the reroll.
Comment #24
dawehnerCan't we declare controllers as a not supported API, instead? It feels really a bit stupid to add
@internal
to all those things.Comment #26
chenderson CreditAttribution: chenderson commentedI am working on this issue at DrupalCon Vienna.
Comment #27
yogen.prasad CreditAttribution: yogen.prasad commentedI am working on this issue with @Valthebald and @chenderson at DrupalCon Vienna.
Thanks
Comment #28
yogen.prasad CreditAttribution: yogen.prasad commentedComment #29
chenderson CreditAttribution: chenderson commentedThis is not a novice issue as per comment Comment #24
Comment #30
tedbowRemoving 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
Comment #31
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #34
mradcliffeThe patch no longer applies so this needs another re-roll and a review to get this in.
Comment #35
imalabyaRe-rolled the patch.
Comment #36
wengerkStill need reroll
Comment #37
wengerkComment #38
wengerkHere 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.Comment #39
wengerkComment #40
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedI'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.
Comment #41
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented#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 forextends 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'.Comment #42
wengerkThanks for your review !
Here the changes from the suggestions in #41
Comment #43
wengerkComment #44
dawehnerI am wondering whether we could add a codestyle rules to automatically check for that.
Comment #46
apadernoI changed the proposed solution to match with the described task and the given patches.
Comment #47
tatarbjComment #48
fabienlyHi, on drupal-8.8.x-dev patch fail to apply on :
Comment #50
shimpyComment #51
shimpy#42 is not getting applied. I would like to work for #41
Comment #52
Priyadarshini Govinthan CreditAttribution: Priyadarshini Govinthan at UniMity Solutions Pvt Limited commentedI have added @internal tag to the controller classes for version 8.9.x. Please review.
Comment #54
apadernoComment #55
shimpyI have created a patch by adding @internal to all the controller classes , methods etc in drupal core with version 8.9.x. Please review.
Comment #56
shimpyI have got phplint failed error for #55. Can anyone suggest me how to correct this error??
Comment #57
shimpyI have re-created a patch for controller doocumentation.
Please review.
Comment #59
shimpyI have tried recreating a patch. Hope this will pass the test.
Comment #60
apadernoThere is a syntax error in the patch.
Comment #61
shimpyI have tried correcting the php parse error.
Comment #63
Dinesh18 CreditAttribution: Dinesh18 at Singapore Press Holdings commentedFixed the failing test. Here is the updated patch.
Kindly review
Comment #64
Dinesh18 CreditAttribution: Dinesh18 at Singapore Press Holdings commentedComment #65
fabienlyComment #67
Gayathri J CreditAttribution: Gayathri J at UniMity Solutions Pvt Limited commentedHii @Dinesh18 #63 not getting applied coming whitespace errors.
Comment #68
ChrisDarke CreditAttribution: ChrisDarke at Hook 42 commentedAdding Amsterdam2019 tag, for work in DrupalCon Amsterdam 2019.
Needs review and testing of patch and fix of the issue in the comment #67.
Comment #69
FrederikvhoI am going to check this out now at DrupalConAmsterdam2019
Comment #70
nickmans CreditAttribution: nickmans at The Reference commentedI'm going to work together with Frederikvho
Comment #71
nickmans CreditAttribution: nickmans at The Reference commentedRemoved all whitespaces. Please review.
Comment #72
FrederikvhoI tested this on 8.9.x and it works. The patch error has been resolved in comment #71.
Comment #73
apadernoMay we just wait for the tests to complete?
Comment #75
nickmans CreditAttribution: nickmans at The Reference commentedFixed coding standards issues, but not sure on how to fix this:
Comment #76
shimpyI tried to fix the test fail error. Not sure if it will work or not.
Comment #77
shimpyComment #79
apadernoThe failing test is the following one.
The controller method that should cause a fatal error is
ErrorTestController::generateFatals()
.Comment #80
apadernoThe 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."
Comment #81
apadernoComment #82
apadernoComment #83
apaderno(The third time should go fine.)
Comment #85
apadernoAs reminder, whenever the ErrorTestController.php file is edited, the following lines could need to be edited too.
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.Comment #86
shimpy#85 looks good.. Cleany applied.
Thanks @kiamlaluno
Comment #87
catchNeeds a 9.0.x patch.
Comment #88
mradcliffeTrackerPage, 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.
Comment #94
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedCreated for Drupal 9.5.x dev version please test and review this.
Comment #95
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedFor 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
Comment #96
SpokjePatch doesn't apply, looks like there's a bit of local work on /vendor/coder included?
Comment #97
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedFor 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
Comment #98
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedFor 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
Comment #99
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedFor 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
Comment #100
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedFor 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
Comment #101
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedFor Drupal 9.5.x dev
https://www.drupal.org/files/issues/2022-07-08/drupal-set-controllers-as-internal-2873668-101.patch
Comment #102
apadernoIt seems the line number is wrong, since that exception message isn't found in the page output.
Comment #103
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedhttps://www.drupal.org/files/issues/2022-07-11/drupal-set-controllers-as-internal-2873668-102.patch
For drupal 9.5.x dev
Comment #104
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #105
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #106
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #107
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #108
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #109
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #110
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedThank 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
Comment #111
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #112
JatinGupta40 CreditAttribution: JatinGupta40 at QED42 for Drupal India Association commentedI will review this patch
Comment #113
JatinGupta40 CreditAttribution: JatinGupta40 at QED42 for Drupal India Association commentedAll the @insert are placed properly. The patch is working fine and updating to RTBC.
Comment #115
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #116
Anjali RathodThe patch applies successfully and works fine. Dont know why it was moved to Needs review again!
Comment #117
xjmGreat 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:
So, we should do something similar for this issue, like:
Comment #118
xjm@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!
Comment #119
xjmThis 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:
Only that last line for
UncaughtExceptionTest
needs to be fixed (again).Comment #120
ChrisDarke CreditAttribution: ChrisDarke commentedThis issue is tagged for first time contributors at DrupalCon Prague 2022.