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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mr.baileys’s picture

Status: Active » Needs review
FileSize
610 bytes

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

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

Steven Jones’s picture

Here's patch that just has the test

Steven Jones’s picture

And then the test + fix

Steven Jones’s picture

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

if (file_exists(DRUPAL_ROOT . '/' . $file)) {
  $files[$type][$name] = $file;
}
Steven Jones’s picture

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

Steven Jones’s picture

FileSize
642 bytes

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.

Steven Jones’s picture

FileSize
1.23 KB

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

Apologies for all the different patch files.

Steven Jones’s picture

According to drupal_web_test_case.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.

mr.baileys’s picture

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

mr.baileys’s picture

Status: Active » Needs work

oops. Status change.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

Okey dokey.

Damien Tournoud’s picture

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

Steven Jones’s picture

Status: Needs review » Needs work

So this needs some attention then.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
3.34 KB

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.

Steven Jones’s picture

Looks good to me.

carlos8f’s picture

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.

Steven Jones’s picture

Status: Needs review » Needs work
FileSize
4.54 KB

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.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.77 KB

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

Steven Jones’s picture

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

Steven Jones’s picture

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?

Steven Jones’s picture

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

catch’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

quicksketch’s picture

Title: drupal_get_path doesn't work for profiles » Test 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.

Steven Jones’s picture

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.

quicksketch’s picture

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.

catch’s picture

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.

chx’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
chx’s picture

FileSize
2.04 KB

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

chx’s picture

FileSize
2.06 KB

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

chx’s picture

FileSize
2.07 KB

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

xjm’s picture

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

webchick’s picture

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.

chx’s picture

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.

chx’s picture

FileSize
2.11 KB

Linked to the issue.

xjm’s picture

+++ 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().

chx’s picture

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.

xjm’s picture

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

chx’s picture

I unfollowed the issue, have fun bikeshedding.

Steven Jones’s picture

Documentation is not bikeshedding, it's essential.

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

Steven Jones’s picture

FileSize
4.69 KB

Damn, forgot to attach it.

Steven Jones’s picture

FileSize
4.84 KB

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

Steven Jones’s picture

FileSize
5.43 KB

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

Steven Jones’s picture

FileSize
5.53 KB

And add some better docs to that script file.

Steven Jones’s picture

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:

drupal_get_filename('script', 'test');

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

Steven Jones’s picture

Issue summary: View changes

issue summary

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

Steven Jones’s picture

Title: Test broken: drupal_get_path doesn't work for profiles » Test 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.

chx’s picture

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.

xjm’s picture

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.

Steven Jones’s picture

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

Steven Jones’s picture

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?

chx’s picture

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

Anonymous’s picture

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.

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

Steven Jones’s picture

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.

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Test broken: drupal_get_path doesn't work for profiles and test coverage is poor » Test 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.

Anonymous’s picture

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

chx’s picture

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.

xjm’s picture

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

xjm’s picture

Status: Needs work » Needs review
FileSize
2.75 KB

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

xjm’s picture

FileSize
2.81 KB

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.

Steven Jones’s picture

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.

Steven Jones’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

Edited the patch not to repeat "this file".

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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

Anonymous’s picture

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

catch’s picture

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.

Steven Jones’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.42 KB

Here's the direct port from the D8 patch.

Steven Jones’s picture

Assigned: xjm » Unassigned
chx’s picture

Status: Needs review » Reviewed & tested by the community

Should be good to go.

boombatower’s picture

Can confirm this fixes for me in 7.x.

webchick’s picture

Title: Test broken: drupal_get_path doesn't work for profiles » drupal_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.

Anonymous’s picture

Issue summary: View changes

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