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
When using a bare PHP install, missing extensions can trigger unclear error messages.
Proposed resolution
PHPUnit provides functionality for documenting and providing feedback to the user about the missing extension with the @requires annotation so we should use is.
Remaining tasks
Patch, Review, Commit.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | use_phpunit_s_requires-2714607-2.patch | 1.27 KB | neclimdul |
Comments
Comment #2
neclimdulcurl is pretty obvious, missing constant error with a CURL_* constant. Still this is going to be clearer.
gd is not obvious unless you're familiar with php's wonky function names and the fact that image_* refers to gd methods.
Comment #3
dawehnerWhat about requiring the CURL php extension in the require-dev section of composer.json? All tests basically rely on CURL, don't they, so adding it to individual classes doesn't really make that much sense.
Comment #4
neclimdulPretty sure only WebTests need CURL as a general rule.
Comment #5
dawehnerThat is a good point! On the other hand we never trigger CURL in those tests, don't we?
On top of that I was wondering whether we could add requirements in BrowserTestBase (as a property or so), but well, sadly this is not actually the case. It just supports annotations, see
\PHPUnit_Util_Test::getRequirementsComment #6
neclimdulNo I don't think we call curl methods in those tests but we do trigger code that uses the constants which is for these purposes the same. If those constants where moved we could easily move these requirements closer to the methods testing curls.
That said, its pretty reasonable to expect to have the curl extension installed since we document its requirement. Going through all that effort probably isn't worth it. Backstory, the curl extension was accidentally uninstalled for me during an upgrade and the tests telling me the constant was missing briefly made me thing something was broken in the tests or a curl was compiled wrong or something instead of immediately realizing the extension was missing. This just improved the DX of the failures by telling you the extension is missing.
For the BrowserTestBase requirement checking, I would suggest using markTestSkipped in setup as documented here:
https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#i...
We do something similar in migrate tests. Its not in setup but a setup helper method for I'm sure good reasons I don't know or remember but its the same idea.
http://cgit.drupalcode.org/drupal/tree/core/modules/migrate/tests/src/Un...
Comment #7
klausiLooks good, I assume we don't need "@requires extension curl" on BrowserTestBase because it use Mink/Goutte/Guzzle which falls back to sockets if curl is not available, right?
Related: #2742541: ApcuBackendTest is skipped in phpunit because of wrong @requires
Comment #8
tstoecklerI still think that makes sense because without it you cannot enable Simpletest module, so I think that fits the "require-dev" semantic pretty well, but maybe that's a separate issue.
Comment #9
klausiIn a post-Simpletest world people will not enable any module when they execute phpunit.
So I think the @requires declaration just on the test classes is fine, otherwise we would also add GD to require-dev in composer.json, right?
Comment #10
neclimdulre #7 Wasn't sure but just disabled curl and that seems to be the case.
re #8 Yeah, it might seem like a generally bad idea to develop without curl but its not actually a requirement. Additionally, we might actually want to run tests without curl since we don't require it for normal sites.
Comment #11
alexpottCommitted 0d4a373 and pushed to 8.1.x and 8.2.x. Thanks!