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:

<?php
$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:

<?php
$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.

Files: 
CommentFileSizeAuthor
#68 drupal-1006714-68.patch2.42 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 36,952 pass(es).
[ View ]
#64 1006714-63.patch2.8 KBchx
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es).
[ View ]
#61 1006714-61.patch2.81 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,820 pass(es).
[ View ]
#60 1006714-60.patch2.75 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,803 pass(es).
[ View ]
#46 drupal-1006714-46.patch5.53 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 33,802 pass(es).
[ View ]
#45 drupal-1006714-45.patch5.43 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 33,808 pass(es).
[ View ]
#44 drupal-1006714-44.patch4.84 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 33,820 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#43 drupal-1006714-42.patch4.69 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 33,808 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#37 1006714-37.patch2.11 KBchx
PASSED: [[SimpleTest]]: [MySQL] 33,807 pass(es).
[ View ]
#33 1006714-33.patch2.07 KBchx
PASSED: [[SimpleTest]]: [MySQL] 33,799 pass(es).
[ View ]
#32 1006714-32.patch2.06 KBchx
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es).
[ View ]
#31 1006714-31.patch2.04 KBchx
PASSED: [[SimpleTest]]: [MySQL] 33,807 pass(es).
[ View ]
#30 1006714.patch1.95 KBchx
PASSED: [[SimpleTest]]: [MySQL] 33,799 pass(es).
[ View ]
#18 drupal-1006714-18.patch4.77 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 31,589 pass(es).
[ View ]
#17 drupal-1006714-17.patch4.54 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 31,534 pass(es).
[ View ]
#16 1006714-16-drupal-get-path.patch4.09 KBcarlos8f
PASSED: [[SimpleTest]]: [MySQL] 30,437 pass(es).
[ View ]
#14 drupal-get-path.patch3.34 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 30,452 pass(es).
[ View ]
#11 drupal-1006714-no-empty.patch1.22 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 30,446 pass(es).
[ View ]
#7 drupal-1006714-fix.patch1.23 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 30,389 pass(es).
[ View ]
#6 drupal-1006714.patch642 bytesSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 30,399 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#3 drupal-1006714-test-and-fix.patch2.22 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 30,444 pass(es).
[ View ]
#2 drupal-1006714-test-only.patch1.62 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 30,363 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#1 profile-dir.patch610 bytesmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 30,401 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new610 bytes
PASSED: [[SimpleTest]]: [MySQL] 30,401 pass(es).
[ View ]

Strange. 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...

<?php
   
// Retrieving a file that is definitely not stored in the database.
   
$this->assertIdentical(drupal_get_filename('profile', 'standard'), 'profiles/standard/standard.profile', t('Retrieve install profile location.'));
?>

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...

StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 30,363 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Here's patch that just has the test

StatusFileSize
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 30,444 pass(es).
[ View ]

And then the test + fix

What's going on here is that the following always evaluates to TRUE I think, if the $file variable is FALSE:

<?php
if (file_exists(DRUPAL_ROOT . '/' . $file)) {
 
$files[$type][$name] = $file;
}
?>

Ah, I didn't realise that there were already tests for this...

StatusFileSize
new642 bytes
FAILED: [[SimpleTest]]: [MySQL] 30,399 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Ah right, BootstrapGetFilenameTestCase subclasses DrupalUnitTestCase, which apparently doesn't really work when doing this sort of testing.

Patch attached that changes the subclass to DrupalWebTestCase.

StatusFileSize
new1.23 KB
PASSED: [[SimpleTest]]: [MySQL] 30,389 pass(es).
[ View ]

And the above (from #6), with the fix in it too.

Apologies for all the different patch files.

According to drupal_web_test_case.php:

<?php
/**
   * Sets up unit test environment.
   *
   * Unlike DrupalWebTestCase::setUp(), DrupalUnitTestCase::setUp() does not
   * install modules because tests are performed without accessing the database.
   * Any required files must be explicitly included by the child class setUp()
   * method.
   */
?>

So that would suggest that any unit test that relies on the database for anything, you should be subclassing DrupalWebTestCase and NOT DrupalUnitTestCase.

Status:Needs review» Active

I 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).

Status:Active» Needs work

oops. Status change.

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 30,446 pass(es).
[ View ]

Okey dokey.

<?php
-class BootstrapGetFilenameTestCase extends DrupalUnitTestCase {
+class
BootstrapGetFilenameTestCase extends DrupalWebTestCase {
?>

Those 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.

Status:Needs review» Needs work

So this needs some attention then.

Status:Needs work» Needs review
StatusFileSize
new3.34 KB
PASSED: [[SimpleTest]]: [MySQL] 30,452 pass(es).
[ View ]

Looking at the function, the tests should cover three scenarios:

  1. bootstrap (database-layer not yet loaded, fetch the information from the file-system)
  2. database (information fetched from system-table)
  3. file system (information not found in system table, fall-back to fetching from file-system)

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.

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.

Looks good to me.

StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 30,437 pass(es).
[ View ]

Neither 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.

Status:Needs review» Needs work
StatusFileSize
new4.54 KB
PASSED: [[SimpleTest]]: [MySQL] 31,534 pass(es).
[ View ]

I'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:

3. file system (information not found in system table, fall-back to fetching from file-system)

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.

Status:Needs work» Needs review
StatusFileSize
new4.77 KB
PASSED: [[SimpleTest]]: [MySQL] 31,589 pass(es).
[ View ]

An 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.)

#18: drupal-1006714-18.patch queued for re-testing.

Issue 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:

  1. bootstrap (database-layer not yet loaded, fetch the information from the file-system)
  2. database (information fetched from system-table)
  3. file system (information not found in system table, fall-back to fetching from file-system)

Basically we need people to review and test the tests themselves, are they enough and do they actually work?

#18: drupal-1006714-18.patch queued for re-testing.

Title:drupal_get_path doesn't work for profilesTest broken: drupal_get_path doesn't work for profiles
Category:bug» task
Priority:Normal» Critical

This 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:

    // Retrieving a file that is definitely not stored in the database.
    $this->assertIdentical(drupal_get_filename('profile', 'standard'), 'profiles/standard/standard.profile', t('Retrieve install profile location.'));

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.

It 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.

Profiles *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.

Category:task» bug
Status:Needs review» Needs work

Since this was already a bug let's leave it as one. This will no longer apply since /core is in.

The current patch is not what we want, someone needs to roll #27 into a patch.

Status:Needs work» Needs review
StatusFileSize
new1.95 KB
PASSED: [[SimpleTest]]: [MySQL] 33,799 pass(es).
[ View ]

StatusFileSize
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,807 pass(es).
[ View ]

Moved into the $files array to avoid subsequent file_exists calls.

StatusFileSize
new2.06 KB
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es).
[ View ]

But if we are doing that then my variable should not be called $filename. Also minor string fixes.

StatusFileSize
new2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 33,799 pass(es).
[ View ]

I forgot to git add after the epic battle with xjm (maybe not) over the comment in drupal_get_filename().

+++ b/core/includes/bootstrap.incundefined
@@ -867,6 +867,11 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+  // Profiles are a special case: they have a fixed location and naming.

My name is xjm, and I approve of this message.

Status:Needs review» Needs work

+++ b/core/scripts/test.script
@@ -0,0 +1 @@
+This file is for testing purposes only.

Don'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.

Status:Needs work» Needs review

No you can't. Observe how the extension is script and the directory is scripts. It's beyond me how else to test this.

StatusFileSize
new2.11 KB
PASSED: [[SimpleTest]]: [MySQL] 33,807 pass(es).
[ View ]

Linked to the issue.

+++ b/core/scripts/test.scriptundefined
@@ -0,0 +1,2 @@
+This file is for testing purposes only. See http://drupal.org/node/1006714 for

Okay, 'nuff IRC reviews 30s after the fact. :) How about: This file is used to test that scripts are properly loaded by drupal_get_filename().

I 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.

I'd at least reference BootstrapGetFilenameTestCase. I think that's the self-documentation that's needed here?

I unfollowed the issue, have fun bikeshedding.

Documentation is not bikeshedding, it's essential.

Here's a patch that takes the fix from #37 and the superior tests from #18.

StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] 33,808 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Damn, forgot to attach it.

StatusFileSize
new4.84 KB
FAILED: [[SimpleTest]]: [MySQL] 33,820 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

And this patch adds chx's test from #37 .

StatusFileSize
new5.43 KB
PASSED: [[SimpleTest]]: [MySQL] 33,808 pass(es).
[ View ]

Here's a patch that adds the DB not being around fix from #18 to the patch from #44.

StatusFileSize
new5.53 KB
PASSED: [[SimpleTest]]: [MySQL] 33,802 pass(es).
[ View ]

And add some better docs to that script file.

So 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:

<?php
drupal_get_filename
('script', 'test');
?>

will search in '/core/scripts' for the file: 'test.script'.

Issue summary:View changes

issue summary

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Title:Test broken: drupal_get_path doesn't work for profilesTest broken: drupal_get_path doesn't work for profiles and test coverage is poor

Updated title to reflect the fact that test coverage is poor before the patch in #43 onwards.

While 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.

I 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.

@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.

To try to move this issue on, the issue summary says that the remaining tasks are:

It's done.

Is that the truth, can we mark this RTBC?

I 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 :(

Status:Needs review» Needs work

i'm not sure the tests exercise the 'no-database' path in drupal_get_filename() for the reasons people think they do.

<?php
   
// Clone the current connection and replace the current prefix.
   
$connection_info = Database::getConnectionInfo('default');
   
Database::renameConnection('default', 'simpletest_original_default');
    foreach (
$connection_info as $target => $value) {
     
$connection_info[$target]['prefix'] = array(
       
'default' => $value['prefix']['default'] . $this->databasePrefix,
      );
    }
   
Database::addConnectionInfo('default', 'default', $connection_info['default']);
?>

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.

Status:Needs work» Needs review

@beejeebus The path forward is clear then:

  1. Commit chx's patch from #37 to fix this critical.
  2. Open another issue to improve the way that drupal_get_filename is tested.
  3. Forget about that issue.

Marking as needs review, as someone needs to review #37.

Issue summary:View changes

Updated issue summary.

Title:Test broken: drupal_get_path doesn't work for profiles and test coverage is poorTest broken: drupal_get_path doesn't work for profiles
Assigned:Unassigned» xjm

Trying to render the relevant info in #54 and #47 into inline comments.

Edit: I totally did not change the title. PI has gremlins.

yes, 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().

That 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.

Status:Needs review» Needs work

So 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:

+++ b/core/includes/bootstrap.incundefined
@@ -883,9 +889,9 @@ function drupal_get_filename($type, $name, $filename = NULL) {
-      if (function_exists('db_query')) {
+      if (class_exists('Database') && Database::isActiveConnection()) {

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. ;)

Status:Needs work» Needs review
StatusFileSize
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 33,803 pass(es).
[ View ]

Alright, 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. :)

StatusFileSize
new2.81 KB
PASSED: [[SimpleTest]]: [MySQL] 33,820 pass(es).
[ View ]

Reviewed with chx again. We want two things for the dummy file:

  1. Non-developers understand quickly that there's nothing to look at.
  2. Developers have the information needed to find out what the file is for.

So, rerolled with two paragraphs.

Status:Needs review» Needs work

@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.

Dreditor is going crazy and changing issue statuses behind my back!

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es).
[ View ]

Edited the patch not to repeat "this file".

Status:Needs review» Reviewed & tested by the community

Alright. I think we're all on board that this is ready once testbot passes.

yep, if the bot is happy, i'd agree this is RTBC.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

This 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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new2.42 KB
PASSED: [[SimpleTest]]: [MySQL] 36,952 pass(es).
[ View ]

Here's the direct port from the D8 patch.

Assigned:xjm» Unassigned

Status:Needs review» Reviewed & tested by the community

Should be good to go.

Can confirm this fixes for me in 7.x.

Title:Test broken: drupal_get_path doesn't work for profilesdrupal_get_path doesn't work for profiles
Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Copy of the revision from November 6, 2011 - 08:47.