The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
When people think of making stuff private, its hard to think of usecases in the future.
One example: \Drupal\Tests\BrowserTestBase::prepareDatabasePrefix ... why would ever someone override it.
Here is a usecase: test an existing site installation rather than setting up an entire Drupal installation.
Proposed resolution
Make everything protected.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 2793443.patch | 1.3 KB | borisson_ |
Comments
Comment #2
dawehnerComment #3
jibranOK
Comment #4
larowlanWhy stop at two tags
Comment #5
xjmLOL @ the tags.
This is in line with our coding standards. Given the status of BTB generally as discussed in other internal API issues, and that 8.2.x is still in beta for a few more days, I've also backported the fix to 8.2.x. Committed bde1906 and pushed to 8.3.x and 8.2.x. Thanks!
Comment #8
dawehnerOne thing which would be nice though is to mark those methods as @internal, aka. this might change in the future.
Is this possible?
Comment #9
alexpottI'm not sure that this change was correct. These methods were protected for a reason. Because individual tests have no business overriding them and if they do they are very much on their own. Additionally tests have no business calling them either.
The stated use case
Needs far more work - since tidying up after a test on an existing site needs a lot more thought.
Comment #10
dawehnerRight, but you need to be able to override some of those methods to even get started with it.
Comment #11
xjmProtected methods are implicitly considered internal and so can change from minor to minor so long as it's not disruptive, but it can't hurt to label them so explicitly, for sure. If they are truly internal, which these seem to be. I don't think we should mark things as internal only because we want to change them (e.g. in the case of things intended as public API) but in this case it makes sense. Followup issue?
I'm confused; they still are protected. They were previously private which was against our coding standards. @dawehner also presented a usecase for them being protected in the summary.
Feel free to revert if it's incorrect, in any case.
Comment #12
dawehnerIs it? I tried to find examples for that: None of them mention anything: https://www.drupal.org/coding-standards/docs https://www.drupal.org/coding-standards https://www.drupal.org/node/608152
For me private methods are totally legit, I mean especially for custom/contrib code, it is fine for using private methods. Core though, as it grew organically over time, has not the best interface design, and by that need often protected methods to allow people to swap out specific bits.
I'd be fine, when we proceed with #2793445: Allow BTB to test an existing, already installed Drupal site instead of installing from scratch as this achieves everything I could imagine for my usecase.
Let's remove the silly tags :)
Comment #13
xjmHm, apparently it was removed here without any coding standards discussion or anything:
https://www.drupal.org/node/608152/revisions/view/2739353/6786689
The explanation is:
It used to say:
If it needs a revert, please reopen the issue. But again, the methods are still protected, not public, so I'm not sure if @alexpott mistyped or missed what the actual change was.
Comment #14
dawehnerI had the discussion with @alexpott in IRC at some point, and yeah this is actually about being private and by that not being touchable. The setup of a test should be separate from all the other crap which is going on a high level.
I actually believe that #2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal() would help us to get there
Comment #15
xjm@dawehner So would you want it reverted then also in favor of #2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal() only? (Which sounds awesome BTW, but might not be backportable to 8.2.x.)
Comment #16
dawehner@xjm
If you are fine with that ...
Comment #19
xjmAlright, reverted. Setting as NR for now to discuss further, close as wontfix, close as dupe of the other issue, whatever is needed. :) Thanks @dawehner.
Comment #25
borisson_#2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal() is committed, but there are still private functions left in KernelTestBase.
Comment #32
smustgrave commentedRunning tests for 10.1
Comment #33
smustgrave commentedPasses 10.1 so LGTM
Comment #34
alexpottI've given how long we've lasted with these as private and that no use-case has come up and force this issue we should close this as a won't fix. The discussion from #19 has never occurred. And Drupal Test Traits is a thing that exists and allows you to run tests against an existing site.