Problem/Motivation
drupal_get_path('profile', 'foo');
does not work.
Proposed resolution
Added a special case into drupal_get_filename
to make it work. It's a special case anyways as profiles can't live in sites/X/profiles as modules / themes / theme engines can. Also fixed a test that was using profiles as something not stored in the database to testdrupal_get_filename
. Instead we abuse drupal_get_filename
capability to find any extension in a plural-named directory and added a dummy test.script
into the existing scripts
directory. We could've created a tests directory and put a single test in it but that would be confusing as it's not a simpletest. So another solution would be to create a testfiles
directory and put a single test.testfile
in it.
Remaining tasks
It's done.
User interface changes
None.
API changes
None.
Original report by Steven Jones
Suppose one has an install profile called: 'stevens_profile' then one might want to do this:
$path = drupal_get_path('profile', 'stevens_profile');
As per the documentation for drupal_get_path. But that doesn't work. Now that profiles are recorded as modules in the system table, you need to do the following:
$path = drupal_get_path('module', 'stevens_profile');
Either the documentation needs to be changed or drupal_get_filename
needs to silently convert the 'profile' to a 'module' when doing the DB query.
Comment | File | Size | Author |
---|---|---|---|
#68 | drupal-1006714-68.patch | 2.42 KB | Steven Jones |
#64 | 1006714-63.patch | 2.8 KB | chx |
#61 | 1006714-61.patch | 2.81 KB | xjm |
#60 | 1006714-60.patch | 2.75 KB | xjm |
#46 | drupal-1006714-46.patch | 5.53 KB | Steven Jones |
Comments
Comment #1
mr.baileysStrange. When I manually run
drupal_get_filename('profile', 'minimal');
it returns bool(false), yet the bootstrap tests (BootstrapGetFilenameTestCase) explicitly test the use of drupal_get_filename with profiles, and succeeds...The patch attached fixes the error for me when manually calling
drupal_get_filename()
, and the tests pass. However, since the same tests pass without this patch applied too, something is off...Comment #2
Steven Jones CreditAttribution: Steven Jones commentedHere's patch that just has the test
Comment #3
Steven Jones CreditAttribution: Steven Jones commentedAnd then the test + fix
Comment #4
Steven Jones CreditAttribution: Steven Jones commentedWhat's going on here is that the following always evaluates to TRUE I think, if the $file variable is FALSE:
Comment #5
Steven Jones CreditAttribution: Steven Jones commentedAh, I didn't realise that there were already tests for this...
Comment #6
Steven Jones CreditAttribution: Steven Jones commentedAh right, BootstrapGetFilenameTestCase subclasses DrupalUnitTestCase, which apparently doesn't really work when doing this sort of testing.
Patch attached that changes the subclass to DrupalWebTestCase.
Comment #7
Steven Jones CreditAttribution: Steven Jones commentedAnd the above (from #6), with the fix in it too.
Apologies for all the different patch files.
Comment #8
Steven Jones CreditAttribution: Steven Jones commentedAccording to drupal_web_test_case.php:
So that would suggest that any unit test that relies on the database for anything, you should be subclassing DrupalWebTestCase and NOT DrupalUnitTestCase.
Comment #9
mr.baileysI think it's safe to drop the "empty()" and just go with
$file && ...
(see patch in #1), as $file will always have a value (FALSE if no record found, a value otherwise).Comment #10
mr.baileysoops. Status change.
Comment #11
Steven Jones CreditAttribution: Steven Jones commentedOkey dokey.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedThose tests work, but they are testing the code path of drupal_get_path() that is designed to work without the database. We still want to test both.
One way of doing that would be to start from a DrupalWebTestCase, first run the tests with the database, and then nuke the connection and rerun the tests.
Comment #13
Steven Jones CreditAttribution: Steven Jones commentedSo this needs some attention then.
Comment #14
mr.baileysLooking at the function, the tests should cover three scenarios:
Not sure if/how that would work, tried with Database::removeConnection in an attempt to nuke the connection but that broke the tests.
What about this: since drupal_get_filename uses function_exists('db_query') to check for the availability of the database, and since there is no way to unload db_query in a DrupalWebTestCase after it's loaded, I suggest just using DrupalUnitTestCase to test scenario 1, and using DrupalWebTestCase to test scenario 2 and 3.
Also removed drupal_static_reset from BootstrapGetFilenameTestCase since drupal_get_filename doesn't use drupal_static.
Comment #15
Steven Jones CreditAttribution: Steven Jones commentedLooks good to me.
Comment #16
carlos8f CreditAttribution: carlos8f commentedNeither HEAD nor #14 test for the 'no database' situation correctly, and in fact drupal_get_path() is broken in its detection of database connectivity also. db_query() exists during a DrupalUnitTestCase, and exceptions are specifically ignored, so the 'has database' path is being run. You can verify this by putting some exit() nonsense in an else { } opposing function_exists('db_query') and run the unit test, nothing happens.
Although the $file change is correct (file_exists() is meaningless if $file is FALSE) it still causes a fallback to file scanning when searching for type 'profile', which is not ideal because an entry likely exists in {system} under type 'module' instead (an odd consequence of profiles being fully-fledged modules, but it's a decision made in D7 that we can't go back on now).
With these things in mind I've modified drupal_get_path() to convert $type=profile to $type=module when doing the query, and use Database::isActiveConnection() which is the proper way to do it in DBTNG. Now the 'no database' test is using the correct code path.
Comment #17
Steven Jones CreditAttribution: Steven Jones commentedI've tidied up the comments on the tests to reflect what they are actually doing now.
We still haven't covered point 3 from above:
Does this issue actually count as critical? If our tests were working correctly, then our code would fail them, which is grounds for the priority being critical.
Comment #18
Steven Jones CreditAttribution: Steven Jones commentedAn attempt at adding some coverage of number 3 (which is kinda covered by 1 once we've established that the fall-through is working, which was the point of the OP.)
Comment #19
Steven Jones CreditAttribution: Steven Jones commented#18: drupal-1006714-18.patch queued for re-testing.
Comment #20
Steven Jones CreditAttribution: Steven Jones commentedIssue summary:
Currently
drupal_get_path('profile', 'some_profile_name')
doesn't return the path to the profile. There was also some incorrect logic about how the function detects if it can access the database.To test:
There are automated tests that test the functionality in a number of situations:
Basically we need people to review and test the tests themselves, are they enough and do they actually work?
Comment #21
Steven Jones CreditAttribution: Steven Jones commented#18: drupal-1006714-18.patch queued for re-testing.
Comment #22
catch#1136820: Allow drupal_get_filename() to work with the installed profile. was duplicate.
Comment #23
pillarsdotnet CreditAttribution: pillarsdotnet commentedRelated issue: #241086: Install profiles cannot use drupal_get_path()
Comment #24
pillarsdotnet CreditAttribution: pillarsdotnet commentedSee also: #1311774: Prevent path functions from using database tables that have not yet been created.
Comment #25
quicksketchThis issue gets a bump in priority now due to #22336: Move all core Drupal files under a /core folder to improve usability and upgrades's intentional breaking of a test. This is a new policy (or in the process of becoming policy) according to #1272266: Strategies for far-reaching core patches.
In the core directory move patch, we intentionally commented out the following test:
This is because profiles cannot currently be located in multiple places anyway. They must live in the root directory under "profiles/". So right now even making drupal_get_path() or drupal_get_filename() work on a profile is silly to begin with, since it can only live in one place anyway. Honestly I think the most straight-forward approach at this time would be to update the documentation to remove the claim that drupal_get_path() works on profiles, since it's unneeded. This functionality would be needed if profiles could live in multiple places, which is the topic of #562042: Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles.
This action may be slightly preemptive considering #22336: Move all core Drupal files under a /core folder to improve usability and upgrades isn't committed yet, but we have to do one thing at a time or we get into a circular blocking pattern.
Comment #26
Steven Jones CreditAttribution: Steven Jones commentedIt seems a bit odd that we should special case profiles in this way, and not allow them to use
drupal_get_path
, especially as the fix is pretty simple.Comment #27
quicksketchProfiles *are* a special case right now though. There's no point in doing a database lookup to find where a profile lives, since you already know it's going to be in /profiles/[profile_name]. I don't disagree that it seems odd that drupal_get_path() doesn't work with profiles, but if it's a special case we should really special case it and just return the string immediately.
Comment #28
catchSince this was already a bug let's leave it as one. This will no longer apply since /core is in.
Comment #29
chx CreditAttribution: chx commentedThe current patch is not what we want, someone needs to roll #27 into a patch.
Comment #30
chx CreditAttribution: chx commentedComment #31
chx CreditAttribution: chx commentedMoved into the $files array to avoid subsequent file_exists calls.
Comment #32
chx CreditAttribution: chx commentedBut if we are doing that then my variable should not be called $filename. Also minor string fixes.
Comment #33
chx CreditAttribution: chx commentedI forgot to git add after the epic battle with xjm (maybe not) over the comment in drupal_get_filename().
Comment #34
xjmMy name is xjm, and I approve of this message.
Comment #35
webchickDon't make some poor slob 12 months from now dig around and figure out what it's testing. Please document it.
Preferably, can we point it at an existing file rather than cluttering up a non-"tests" directory with this kind of stuff? For example, MAINTAINERS.txt intead.
25 days to next Drupal core point release.
Comment #36
chx CreditAttribution: chx commentedNo you can't. Observe how the extension is script and the directory is scripts. It's beyond me how else to test this.
Comment #37
chx CreditAttribution: chx commentedLinked to the issue.
Comment #38
xjmOkay, 'nuff IRC reviews 30s after the fact. :) How about: This file is used to test that scripts are properly loaded by drupal_get_filename().
Comment #39
chx CreditAttribution: chx commentedI feel linking here is simply better. There's a lot to tell like why was this file necessary in the first place and I really see no reason to squeeze it into this file.
Comment #40
xjmI'd at least reference
BootstrapGetFilenameTestCase
. I think that's the self-documentation that's needed here?Comment #41
chx CreditAttribution: chx commentedI unfollowed the issue, have fun bikeshedding.
Comment #42
Steven Jones CreditAttribution: Steven Jones commentedDocumentation is not bikeshedding, it's essential.
Here's a patch that takes the fix from #37 and the superior tests from #18.
Comment #43
Steven Jones CreditAttribution: Steven Jones commentedDamn, forgot to attach it.
Comment #44
Steven Jones CreditAttribution: Steven Jones commentedAnd this patch adds chx's test from #37 .
Comment #45
Steven Jones CreditAttribution: Steven Jones commentedHere's a patch that adds the DB not being around fix from #18 to the patch from #44.
Comment #46
Steven Jones CreditAttribution: Steven Jones commentedAnd add some better docs to that script file.
Comment #47
Steven Jones CreditAttribution: Steven Jones commentedSo in case someone arrives at this issue wondering what core/scripts/test.script is all about, here's some more information.
When drupal_get_filename fails to find a file in the database cache in the system table, it tries to find a file on the filesystem by manually searching.
Files of type 'script' are never cached in the database, so we use that type to ensure that the database lookup will always fail.
When searching the filesystem, drupal_get_filename looks in a few specific directrories, one of which is '/core'. So we exploit the fact that we already have a 'scripts' directory, so that:
will search in '/core/scripts' for the file: 'test.script'.
Comment #47.0
Steven Jones CreditAttribution: Steven Jones commentedissue summary
Comment #47.1
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #47.2
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #48
Steven Jones CreditAttribution: Steven Jones commentedUpdated title to reflect the fact that test coverage is poor before the patch in #43 onwards.
Comment #49
chx CreditAttribution: chx commentedWhile documentation is essential look at where we have arrived after, yes, bikeshedding the contents of a file which is never used: my version was "This file is for testing purposes only". No need to read further if you just wonder wtf is that. If you are really curious you can open an issue.
Now read "+This file is used to test that 'scripts' are properly located by..." this is a complex sentence that's a lot harder to understand. There is no clear indication to "keep out", it's not even clear any more whether there is any other purpose etc.
But, have fun holding up a critical over nothing.
Comment #50
xjmI thought you unfollowed the issue. ;)
It's been less than a day since chx's patch in #30, and this issue has been open literally since last Christmas. I don't think it's quite fair to say it's being "held up" by webchick or Steven Jones or myself. You say "bikeshed," I say "iterative improvement that increases the likelihood of your fix being committed quickly." Core is about collaboration, after all; this is how we (re)roll.
Edit: I'd add my cleanup to the patch, if I didn't feel like I was going to rouse other contributors' ire by so doing.
Comment #51
Steven Jones CreditAttribution: Steven Jones commented@chx I added your final version of that file in #45, in addition to a more verbose version in #46.
Someone else can decide which they want to commit, or if they want a new version (maybe in a follow-up patch.)
As for 'holding up' an issue, I spent about 60 minutes on this issue, and plus time for the test bot, I would hardly call that a huge hold up.
Comment #52
Steven Jones CreditAttribution: Steven Jones commentedTo try to move this issue on, the issue summary says that the remaining tasks are:
Is that the truth, can we mark this RTBC?
Comment #53
chx CreditAttribution: chx commentedI am sorry for calling this bikeshedding. Really sorry. I shouldn't be patching core at all as I lack the patience these days to do it but I can't leave the critical queue alone when it's above the treshold :(
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm not sure the tests exercise the 'no-database' path in drupal_get_filename() for the reasons people think they do.
we totally have an active db connection for unit tests. but:
- it's prefixed to non-existent tables, so it won't return you anything
- Database::isActiveConnection() will return false *only if you call it before attempting any queries*. which is umm, ungood, but Crell thinks Database::isActiveConnection() is "vestigial code" anyway
not 100% of the way forward, but i hate to see us put this in based around an accidentally correct test.
Comment #55
Steven Jones CreditAttribution: Steven Jones commented@beejeebus The path forward is clear then:
drupal_get_filename
is tested.Marking as needs review, as someone needs to review #37.
Comment #55.0
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #56
xjmTrying to render the relevant info in #54 and #47 into inline comments.
Edit: I totally did not change the title. PI has gremlins.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedyes, i agree with #55 - let's go with chx's patch, because it fixes a real issue, and open another issue if people want to pursue the changes related to Database::isActicveConnection().
Comment #58
chx CreditAttribution: chx commentedThat fix was incorrect anyways because your problem is not having a database connection. Even if you have a DB connection, Drupal might not yet be installed, the DB table can be crashed and all sorts of other nastiness. The unit test covers this scenario as the table is prefixed to something nonexisting.
Comment #59
xjmSo I spoke to chx and beejeebus about this on IRC again, and we agree that we really should not introduce this change in this issue:
So, I'm going to go back to #37, add Steven's documentation and some other comments, and we should defer the additional test coverage and what might be needed for it to those followup issues. ;)
Comment #60
xjmAlright, I tried to condense the key facts here into an inline comment in the test so we can avoid having to make people read this whole issue to understand WTF we're doing. I certainly didn't before. :)
Comment #61
xjmReviewed with chx again. We want two things for the dummy file:
So, rerolled with two paragraphs.
Comment #62
Steven Jones CreditAttribution: Steven Jones commented@xjm Perfect.
I've spun out the additional tests I wrote ages ago into a new issue, over here: #1334862: Test broken: drupal_get_path doesn't work for theme engines and test coverage is poor, I would appreciate help in fixing
drupal_get_filename
over there, if you have a few minutes. The tests are pretty simple, and if they were in core, there would be an additional critical issue to fix.Comment #63
Steven Jones CreditAttribution: Steven Jones commentedDreditor is going crazy and changing issue statuses behind my back!
Comment #64
chx CreditAttribution: chx commentedEdited the patch not to repeat "this file".
Comment #65
xjmAlright. I think we're all on board that this is ready once testbot passes.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, if the bot is happy, i'd agree this is RTBC.
Comment #67
catchThis has had plenty of review and looks great to me. I was a bit put of by core/scripts/test.script since it's adding cruft to a core directory, but xjm explained we can only use a directory with the extension named the same as the dir since that's how drupal_get_filename() works, and I don't see away around this. If we're going to choose a location to put a dummy file, then /scripts makes sense since it is all one-off scripts anyway.
Committed/pushed to 8.x, moving to 7.x.
Comment #68
Steven Jones CreditAttribution: Steven Jones commentedHere's the direct port from the D8 patch.
Comment #69
Steven Jones CreditAttribution: Steven Jones commentedComment #70
chx CreditAttribution: chx commentedShould be good to go.
Comment #72
boombatower CreditAttribution: boombatower commentedCan confirm this fixes for me in 7.x.
Comment #73
webchickCommitted and pushed to 7.x. Thanks!
Comment #74.0
(not verified) CreditAttribution: commentedCopy of the revision from November 6, 2011 - 08:47.