Fatal error: Call to undefined function features_get_components()

The patch assumes that features.module will be in the include path, which is not necessarily the case : better use the actual path since we know it.

It is not exactly the same. Quoting the php doc: "Files are included based on the file path given or, if none is given, the include_path specified. If the file isn't found in the include_path, include will finally check in the calling script's own directory and the current working directory before failing. The include construct will emit a warning if it cannot find a file; this is different behavior from require, which will emit a fatal error.".

...meaning that if the file can be found anywhere else in the include path (normally sane, but not necessarily), the other copy will be included it include_path does not start by ".", because it looks in the directory of the including file after the include path, not before. It also probably means a number of disk accesses to check for the file existence in each directory in the include path, all of which are avoided by specifying the absolut path.

But, granted, in /most/ situations, the basic version will work normally.

The patch in #4 allowed me to uninstall Features. Thanks.

I can also confirm that the patch in #4 resolves this problem.

I can confirm, the patch in #4 allowed me to uninstall Features. Thanks.

Patch #4 works for me.

Patch #4 works fine for me too.

#4 works for me too. By the way, what about to include this patch into next release?

Patch #4 is great. Please include it in the next module release. Thanks @Chi and @fgm! This was about the quickest I've been able to resolve an issue like this.

Patch #4 worked for me too.

Patch #4 worked fine.

https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup... drupal_load would likely be better to call

Patch #4 worked for me too.

Thanks, #4 worked like a charm for me.

Reviewed
fixed it for me to merge this patch(line)

Thanks for the patches/reviews. I should have set this to needs work earlier.

I'm not going to commit this in present state as (to my knowledge) it's not a standard way to include a .module file, and when can, should follow the standard way of doing things to make developer experience best. So, show another place in core that includes a .module via that method (require_once etc) or change to drupal_load() (or whatever the standard way to include a .module is if it's not that);

@hefox it is a standard way to include a file in PHP. As of >= PHP 5.3 it's __DIR__ instead if dirname(__FILE__).

It's safer to do this method than rely on drupal's bootstrapped includes and technically, though very very slightly, faster.

We are doing it a bit more in D8, though still mostly with .inc files because we aren't looking for module functions often:

core/modules/simpletest/tests/src/Unit/PhpUnitErrorTest.php

19    public function testPhpUnitXmlParsing() {
20:     require_once __DIR__ . '/../../../simpletest.module';

Thanks. What you're saying is this is a way used in drupal 8 but don't have any drupal 7 examples/usage? Features is drupal 7 and should comply with how drupal 7 is done.

Why would relaying on bootstrap be problematic during a module uninstall?

D7 core examples:

/includes/database/schema.inc:
    6   */
    7  
    8: require_once dirname(__FILE__) . '/query.inc';
    9  
   10  /**

includes/database/select.inc:
    6   */
    7  
    8: require_once dirname(__FILE__) . '/query.inc';
    9  
   10  /**

/includes/filetransfer/filetransfer.inc:
  297      }
  298  
  299:     $path = dirname(__FILE__);
  300      $path = $this->fixRemotePath($path, FALSE);
  301      $parts = explode('/', $path);

/modules/field/field.info.inc:
   25      // a couple upgrade tests (DisabledNodeTypeTestCase,
   26      // FilterFormatUpgradePathTestCase...) break in a strange way without it.
   27:     include_once dirname(__FILE__) . '/field.info.class.inc';
   28      $field_info = new FieldInfo();
   29    }

If the module isn't loaded (because it's being uninstalled and disabled) the module's file is also not loaded. Depending on the bootstrap phase it may not have the files necessarily loaded to do an include the 'drupal way'.

module_load_include() on the API page is not supposed to be used in an *.install file. @see https://api.drupal.org/api/drupal/includes!module.inc/function/module_lo...

The alternative module_load_install() would be silly to call when trying to uninstall the module.

I'm not changing the status again... I think this is still RTBC.

Why would a module be being put through module uninstall when not at fully bootstrapped? That seems like asking for problems, considering the module uninstall is now hook-able. considering that, I don't see any reason not to use drupal_load, which seems to be the standard way to load a module, which is what seems to be desirable here.

@hefox I could be totally wrong... maybe you are right. Although it sounds like any other way than including the file manually once to get access to it's methods during an uninstall would cause more trouble than it's worth.

You're likely right, bootstrap has nothing to do with this here. I'm just thinking that could be an issue with drush or some other tool for disabling modules. It's more that the module is disabled so it's a bit of a chicken and egg thing if we load it while it's trying to remove itself. Edit not helpful.

drupal_load() like you mentioned above would likely work. It's doing a lookup all over the filesystem OR staticly keeping track of them if it's been run before on the same request, so it would be really slow or just a few ms slower.

So it may be a bit overkill when you want to include one file in the same directory, no?

I suspect if someone is running uninstall without bootstrap, something is broken, cause the module then cannot be properly uninstalled.

My main motivation is that I do not like tying things to directory structure unless have to -- e.g. the code is copy and pasted elsewhere, it'll likely still work.

drupal_load is just a db_query in a fully installed system. The file system stuff is for if called during really early site install

// The database table may not exist because Drupal is not yet installed,
// or the database might be down. We have a fallback for this case so we
// hide the error completely.

And my motivation is simplicity and performance mostly. Though a bit of micro-optimization I admit, one of drupals worst performance hits comes when many modules need to be traversed in the file system. Also the path is relative so it doesn't matter where the files is moved. If you use drupal_load() and moved the file you'd have the same change to make. *Relative to the modules folder, where is the file?*

It's your call though, you are the maintainer and the change is minor either way. Just needs a solution in.

I understand your motivation; performance is important, but for "one time" actions like module install, robust code > performance. Then there's this: #1068932: Use include instead of include_once() in drupal_load()

Yeah the performance is more an exercise in education, getting people to realize the performance implications of the functions they use for convenience or convention. Thanks for pointing me at that issue, very interesting!

Actually, robustness is very well enforced when using __DIR__ (if 5.3 is required) or dirname(__FILE__), which are faster than drupal_load().

The reason why drupal_load() is needed for robustness is when referencing files outside the current module : in that case, one cannot know where the other file is present, and drupal_load() is needed to resolve this.

However, within a single module, file locations are known and immutable, so the extra work performed by drupal_load() can be avoided, probably like:

if (!function_exists('features_get_components')) {
  require dirname(__FILE__) . '/features.module';
}

This avoids both the drupal_load() extra work and the filesystem (NFS) issues associated with require_once(), making it probably even a bit faster than my #4 patch.

#31 sounds good to me. I'd RTBC that:)

With the mentioned include vs include_once patch would prevent #31, as a later call to drupal_load would fatal. That drupal_load patch is optimizing bootstrap, a place where performance improvements effects a helluvalot. This patch is for uninstalling, where the minimal difference (miliseconds?) between the difference approaches isn't worth not using the relevant api functions.

@mpotter or other maintainers might have a different opinion, but I plan to commit #30 after testing next time I'm commiting to features.

So, been thinking about this patch again.

The point of calling that function is to get all components so can delete the variables.

But that'll miss any components that are part of disabled modules.

Personally, it always seem more thorough to query the table for name spaced variables (e.g. ones that begin with variable name features_component_locked_, etc. ) and delete that. Opinions?

That sounds like a much better solution.

Does this look right?

db_delete('variable')
  ->condition('name', 'features_admin_show_component_%', 'LIKE')
  ->execute();
db_delete('variable')
  ->condition('name', 'features_component_locked_%', 'LIKE')
  ->execute();

From a quick glance, that looks correct but haven't used dbtng with like statements too much

Even better to use just one query to save a round-trip to the DB and a query construction.

Instead of receiving a fatal error I ended up with WSOD. Patch #4 helped, thanks. Features 7.x-2.3 and 2.x-dev.

Re #37 dbtng's db_or is a minor DX pain so I opted for two statements.

#4 solved it for me

Can someone submit a patch for #35.

Right now #30 is the best patch. I agree with @hfox on the use of drupal_load vs require_once. I don't mind require_once at the top of code files as in the examples in #24, but I don't like seeing it in the middle of inline code. #35 seems a better approach to the entire problem.

Here's the patch from #35

Thanks. Committed this fix to 6e7b6a7.