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.
Problem/Motivation
Working on coding standards for the Views module here: #2385209: Clean-up views module test members - ensure property definition and use of camelCase naming convention
We encountered some commented-out code in Drupal\views\Tests\ViewStorageTest::displayMethodTests()
with a @todo
and no issue.
This is that issue.
The lines in question look like this, bemoaning the fact that View::generateDisplayId() is protected:
// Tests Drupal\views\Entity\View::generateDisplayId().
// @todo Sadly this method is not public so it cannot be tested.
// $view = $this->controller->create(array());
// $this->assertEqual($view->generateDisplayId('default'), 'default', 'The plugin ID for default is always default.');
// $this->assertEqual($view->generateDisplayId('feed'), 'feed_1', 'The generated ID for the first instance of a plugin type should have an suffix of _1.');
// $view->addDisplay('feed', 'feed title');
// $this->assertEqual($view->generateDisplayId('feed'), 'feed_2', 'The generated ID for the first instance of a plugin type should have an suffix of _2.');
Proposed resolution
Use reflection to allow test access to generateDisplayId()
.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Unfrozen changes | Unfrozen because it only improves automated tests. |
---|
Comment | File | Size | Author |
---|---|---|---|
#1 | 2413217_1.patch | 2.26 KB | Mile23 |
Comments
Comment #1
Mile23And a patch, too.
Comment #2
Mile23Comment #3
Mile23Still applies.
Comment #4
dawehnerAfaik we do that in multiple places now, so I think we should just accept that its useful to test protected methods and put that into our testing framework somehow.
Comment #5
Mile23If there's a top-level method named something like
makeThisMethodAccessible($object, $method_name)
, you still end up having to remember you have it, and then find out if it does some side-effect you don't want.It's easier to just write the code for reflection in-line, and then it's visible for everyone to understand.
The only down-side to inline reflection like this would be if the behavior of
ReflectionMethod
changed, you'd have to change it a lot of places. I doubtReflectionMethod
will change though.Comment #6
dawehnerIt is certainly not worth fighting for.
Thank you for writing the patch!
Comment #7
alexpottCommitted 935516d and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.