Paths given to spl_autload_register() are relative to Drupal root for module's defined classes.

That is, it will rarely be an issue but it happens that I have a project where I must use Drupal APIs outside of normal Drupal control flow and outside of Drupal root.

CommentFileSizeAuthor
#1 2162691-1.patch1.32 KBdrikc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drikc’s picture

drikc CreditAttribution: drikc commented
Status: Active » Needs review
FileSize
2162691-1.patch1.32 KB

The patch give absolute path for module's defined classes.

Status: Needs review » Needs work

The last submitted patch, 1: 2162691-1.patch, failed testing.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Nice catch.
We want to reproduce the behavior of drupal_get_filename(), which does in fact prepend a DRUPAL_ROOT.

The patch seems to go in the right direction. We need to have a look at the failing tests, but I am not even sure that they pass without this patch :)

We also need to have a look at the 7.x-4.x branch. The problem should no longer occur there since this branch uses drupal_get_filename(). But better have a closer look.

Btw, how do you use Drupal APIs outside of Drupal?
(and, just saying, don't put that apostrophe in "API's" :) Plural don't get an apostrophe.)

donquixote’s picture

donquixote CreditAttribution: donquixote commented

And btw, totally unrelated: I am always looking for people who can give me feedback about the 7.x-4.x API design!

drikc’s picture

drikc CreditAttribution: drikc commented
Issue summary: View changes
drikc’s picture

drikc CreditAttribution: drikc commented

Just made a quick test with 4.x but module defined classes weren't loaded outside of "normal Drupal control flow" so I suspect the include path isn't absolute also...

To respond to comment #3, 4th paragraph: I use the following snippet as an include for an application to be run in a child folder of Drupal root:

if (!defined('XAPP_ROOT')) {
  define('XAPP_ROOT', getcwd());
}
define('XAPP_BASE_PATH', pathinfo($_SERVER['PHP_SELF'], PATHINFO_DIRNAME));
define('DRUPAL_BASE_PATH', preg_replace('/(\/[^\/]+).+$/', '$1', XAPP_BASE_PATH));
define('DRUPAL_ROOT', $_SERVER['DOCUMENT_ROOT'] . DRUPAL_BASE_PATH);
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
// $base_url (a global drupal variable) must be set with the drupal root
// (eg.: http://localhost/drupal) otherwise the Drupal session won't be
// recognized.
global $base_url;
$base_url = (array_key_exists('HTTPS', $_SERVER) ? 'https://' : 'http://') . $_SERVER['HTTP_HOST'] . DRUPAL_BASE_PATH;
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
donquixote’s picture

donquixote CreditAttribution: donquixote commented

Interesting, thanks for sharing.

Yesterday I had a look at drupal_get_filename() and drupal_get_path(). It turns out that these methods return relative paths, but module_load_include() prepends the DRUPAL_ROOT . '/' before including. So this is also what we need to do here.

I am going to fix this asap in both branches.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Release: 7.x-3.5.
Commit: http://drupalcode.org/project/xautoload.git/commitdiff/5ee4b7d78d0b8a64e...
Test results are confusing, but I think this is more due to the tests than the change. I am not going to do big changes to XAutoloadWebTest in the 7.x-3.x branch anymore. It is good enough if it works for real-world sites.

This still needs to be fixed in 7.x-4.x, so until then the issue stays open.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Title: Relative paths given for autoload module's classes includes » Relative paths given for autoload module's classes includes (Follow-up)
Category: Bug report » Task

And finally fixed in 7.x-4.0-alpha4.

Follow-up: It would be nice to have unit tests for this.
Since the usual Drupal web tests take forever, I would prefer to emulate the scenario with phpunit. There is already a tests/ folder and a phpunit.xml.dist, so we only need to add one more test in tests/lib/.

@drikc, your help would be much appreciated!

drikc’s picture

drikc CreditAttribution: drikc commented

@donquixote, thanks for those commits.

I will look into 4.x phpunit ASAP!

Best 2014!

drikc’s picture

drikc CreditAttribution: drikc commented

I've went into this phpunit (found tiny issue #2171641: Hardcoded paths in DiscoveryTest::testWildcardFileFinder()) and could see that the only none phpunit test cannot be moved to phpunit as they depend on a drupal instance: XAutoloadWebTestCase

Which scenario are you referring in you previous comment (#9)?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

@drikc:
The idea is to test as much as we can with phpunit, but still leave some Drupal web tests for stuff where phpunit does not work.
Maybe we can even test some Drupal-specific behavior with phpunit, by mocking the Drupal framework and global state.
But we should still keep the WebTest around that work with REAL Drupal, because the mocked world is not 100% trustworthy.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Title: Relative paths given for autoload module's classes includes (Follow-up) » Unit tests: Relative paths given for autoload module's classes includes (Follow-up)
donquixote’s picture

donquixote CreditAttribution: donquixote commented

We have some nice new PHPUnit tests now in 7.x-5.x, but they do not (yet) properly distinguish relative paths from absolute paths.
In fact I simplified the DRUPAL_ROOT stuff away, out of laziness.

Maybe you could have a look?

drikc’s picture

drikc CreditAttribution: drikc commented

I would like to but I can't figure where or what does mean to "properly distinguish relative paths from absolute paths" in the tests files I've read through (BasicIntegrityTest.php ClassFinderAdapterTest.php ClassLoaderTest.php DiscoveryTest.php)?

donquixote’s picture

donquixote CreditAttribution: donquixote commented

Have a look at the VirtualDrupal stuff.

drupal_get_filename() / drupal_get_path():
http://cgit.drupalcode.org/xautoload/tree/tests/src/VirtualDrupal/Drupal...
which is based on
https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup...
These both return a path relative to drupal root.

The file_exists() checks sometimes use the relative path, but sometimes they prepend DRUPAL_ROOT.
(I think even Drupal core is inconsistent here)

drupal_load():
http://cgit.drupalcode.org/xautoload/tree/tests/src/VirtualDrupal/Drupal...
which is based on
https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup...

    if ($filename) {
      include_once $filename;

vs

  if ($filename) {
    include_once DRUPAL_ROOT . '/' . $filename;

Since the PHPUnit tests are completely independent of Drupal, there won't be any DRUPAL_ROOT defined.
So it probably will be all relative to the folder where you start phpunit.

Solution can be to pass a $drupal_root string around to all VirtualDrupal components.
So, it would be

    if ($filename) {
      include_once $this->drupalRoot . '/' . $filename;

Ideally this parameter should be something non-empty, so that whenever something does not use this string, the test will fail.
See the DIC here.
http://cgit.drupalcode.org/xautoload/tree/tests/src/VirtualDrupal/Drupal...

donquixote’s picture

donquixote CreditAttribution: donquixote commented

If this is all too much for you, I can do it myself when I have time :)

donquixote’s picture

donquixote CreditAttribution: donquixote commented

But sometimes it is nice to have someone else struggle with the same code, to get a 2nd opinion and get out of the brain island.

drikc’s picture

drikc CreditAttribution: drikc commented

I will try (sorry for reacting with such delay),

So, I guess there is no way to determine this $drupal_root parameter automatically in the phpunit context, thus it must be given to phpunit as config (in phpunit.xml) or a command line parameter variable.

Then, once this $drupal_root is defined we can rewrite Drupal\xautoload\Tests\Example\AbstractExampleModules::getExtensionFilename() so that it return filenames relative to this drupal root. And, finally we should prefix some Drupal\xautoload\Tests\VirtualDrupal::drupalGetFilename(...) calls with this same $drupal_root. Right?

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Needs work » Postponed (maintainer needs more info)

Does this issue still apply in 7.x-5.x?
I expect the answer is no.