fubhy brought up a good point that it doesn't look like Composer Manager's autoload.php is loaded in hook_boot() or hook_init()? Would be nice if it was so that modules wouldn't have to worry about loading it themselves.

An example is in the Monolog module:

/**
  * Page callback; Lists the handlers than can be added.
  */
 function monolog_handler_page($profile) {
   $build = array();
 
   composer_manager_register_autoloader();
   $handler_info = monolog_handler_info_load_all();

Having autoload.php loaded in hook_init() or boot would save the composer_manager_register_autoloader() call.

CommentFileSizeAuthor
#17 composer_manager-1955386-17.patch477 bytescpliakas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

This is a tough one. Assuming composer.json is built when it hasn't could result in bringing your site down as the class loader wouldn't be there.

cpliakas’s picture

This is a tough one.

Based on my past experience with Search Lucene API I vowed never to put an autoloader in hook_init() again :-). I could be convinced otherwise, though.

As Rob mentioned, if the autoload.php file isn't available then your application will throw fatal errors if it attempts to use any libraries without any defensive coding. This is an issue because you cannot run composer install until after composer_manager is installed, so there will always be some amount of time where the autoload.php file is unavailable. If another module, for example Git Wrapper, is installed along side Composer Manager, then it should have the ability to catch exceptions thrown when the autoload.php file is not available so it can handle the error gracefully without blowing up the whole app. It could not do this if the autoloader was registered in hook_boot() or hook_init() because it would fail hard. Even if it failed silently, Git Wrapper would fail hard unless it re-detected that the autoload.php file was available, thus negating the goal of just being able to use the libraries.

In addition, hook_init() isn't called during update.php so you have to explicitly call it every time one of the libraries might be used. This was a pain with SLAPI because it implemented some hooks that sometimes got called in hook_update_N() implementations by modules that knew nothing about SLAPI. Therefore SLAPI had to register it's autoloader anyways, regardless of the fact that it was registered in hook_init().

With all that being said, there is nothing stopping a contributed module from calling composer_manager_register_autoloader() in it's hook_init() implementation. That function can be called 100 times at whatever point in the application flow without harm. It will only register a single autoloader, and is uses a normal static in favor of drupal_static() so it is pretty quick.

Fun problems,
Chris

donquixote’s picture

Category: task » feature

composer install until after composer_manager is installed, so there will always be some amount of time where the autoload.php file is unavailable.

Just saying, autoload is only one part of the problem.
If composer install did not run yet, then the library is not there, neither with nor without autoload.

It's a chicken-and-egg situation.

EDIT: I might have some ideas about the autoload, but we need to solve the chicken-and-egg first.

donquixote’s picture

cpliakas’s picture

So based on the chicken & egg discussion, I think we can assume that the module comes first based on how Drupal currently works unless we want to undergo an effort to override a bunch of things to change that, which I am not sure I have the stomach for.

Ruling out hook_boot()

Looking into this further, we can probably rule out hook_boot() since it is invoked in the DRUPAL_BOOTSTRAP_PAGE_CACHE phase which is run before the DRUPAL_BOOTSTRAP_VARIABLES and DRUPAL_BOOTSTRAP_DATABASE phases. Since the location of the vendor directory is configurable, we would need to get really creative to retrieve the path stored in the "composer_manager_vendor_dir" variable.

Would we really save a function call with hook_init()?

Let's say that we move composer_manager_register_autoloader() to hook_init(). As mentioned above there are times when hook_init() isn't invoked, so we would have to take that into account in certain situations anyways. One situation is in hook_update_N() implementations, and another use case is with Monolog (more specifically Monolog Logging) where watchdog() could be called before a full Drupal bootstrap. See #1997462: monolog() function not found for more details. So with that module we have to register the autoloader anyways unless we want to invoke hook_init() which is probably not the best thing to do.

But ignoring that challenge, let say that hook_init() is invoked and it is time to instantiate a class. If the autoloader is not registered and $object = new MyPackage\MyClass() is executed, a fatal error is thrown since the class is not available. I think we can all agree that this is not acceptable. You could us a if (class_exists('MyPackage\MyClass')) condition, but you don't know if that class doesn't exist because the autoloader isn't loaded or Composer's update command hasn't been run to pull in the library.

In Monolog we use composer_manager_register_autoloader() and if (class_exists('MyPackage\MyClass')) which sucks from a DX perspective, but it really allows the application to handle things gracefully and also tells us whether the class isn't available because the autoloader isn't available or Composer update hasn't been run to pull in the dependencies. In tandem with the work done at #2000978: Add a requirements check to see if packages need to be installed, Composer Manager does a pretty good job at hitting the developer and site administrator over the head with the state of the libraries and what needs to be done to ensure everything is available.

Alternatives?

I am open to alternatives, but based on the points above I don't think hook_boot() is viable nor do I think that hook_init() actually prevents us from having to do a sanity check prior to instantiating a class / loading a library. Again, I am open to alternate viewpoints, but they do have to address all the challenges listed above. I will leave this issue open for a while for people to comment, but in absence of alternatives I am going to close this as won't fix.

Thanks,
Chris

cpliakas’s picture

Status: Active » Postponed (maintainer needs more info)

So to sum up the challenges:

With hook_boot()

How do we get the configured vendor directory when neither the database nor variables are available?
What happens when the autoloader is not available and a class is instantiated?

With hook_init()

How do we handle the situations where hook_init() hasn't been invoked?
What happens when the autoloader is not available and a class is instantiated?

donquixote’s picture

From _drupal_bootstrap_page_cache()

  // Check for a cache mode force from settings.php.
  if (variable_get('page_cache_without_database')) {
    $cache_enabled = TRUE;
  }
  else {
    drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES, FALSE);
    $cache_enabled = variable_get('cache');
  }

I am not sure if anyone actually uses that setting.
In xautoload I am going even before hook_boot(), the moment that the xautoload.module file is included. So far no problems have been reported about that (which does not mean they can't happen).

I suspect that neither the inclusion of modulename.info nor the invocation of hook_boot() can ever work without the database. Because how else can Drupal know the location of the module file, and whether the module is enabled?

It could only work if there is some magical 3rd party cache that tells Drupal where modules are. I have not heard of such a thing yet. If it exists, it probably needs a lot of custom stuff going on in settings.php, so then they can also do a bit of extra stuff to get composer going.

cpliakas’s picture

I see. So in hook_boot() you can get variables if they are defined in settings.php. I would want to avoid this installation step if possible, however other caching backends require it so I am not opposed to this. It would also only need to be added if people want to change the defaults, so it wouldn't always be a requirement to add things to settings.php.

So maybe hook_boot() isn't off the table for this. The only problem remaining is error handling. If we add this in hook_boot(), there is still the problem where the autoloader is available but the library is not. I'd be interested in heading approaches on how to handle this scenario.

P.S. There are a lot of parallels in regards to error handling in the discussions at #1992896: What recovery options do we have in case of RuntimeException..

donquixote’s picture

So in hook_boot() you can get variables if they are defined in settings.php.

No, it is the other way:
With an unmodified settings.php, database and variables will be available in hook_boot(), and even when the *.module files are loaded (for boot modules and regular modules).

To stop this behavior, you need to set page_cache_without_database in settings.php (because at the time that is checked, the db-based variables are not there yet). Otherwise, at the time this variable is checked, it will be NULL. (see snippet in #7)

So, this is only ever going to be a problem for people that already do some fancy stuff in their settings.php - which might have a number of side effects beyond just composer_manager autoload.

donquixote’s picture

So, what you could do to really be as early as possible:
1. Make it a boot module by implementing hook_boot(). This can be an empty hook implementation.
2. Fire up the class loader directly from composer_manager.module, not from within any function.
3. Set module weight from hook_install(). (I am not exactly sure if you need it though)
4. Register additional namespaces as soon as new packages are installed. This could be in the middle of a request. (I am not sure how exactly you would do this with the composer class loader, but there must be a way..)
5. People who use page_cache_without_database shall make a separate feature request.

You may not need all of this, but I think this is the most you can get. This is also what xautoload does.

RobLoach’s picture

If Composer Manager used hook_load() or hook_boot() to load autoload.php, modules could do certain checks like class_exists() and such to ensure the correct functionality is available.

donquixote’s picture

#11 Yes. But I think it would be more robust to have a dedicated function to tell about the status of a package.
With class_exists() it can always happen that some other module has included a specific file, but the autoload is still not in place.

Dave Reid’s picture

I have to admit it was a bit of a DX WTF to get Composer Manager set up and downloading all the required packages, and then I went to devel/php to see if I could load the classes and they all returned class_exists() == FALSE.

So I find this statement on the project page a bit misleading because to me it means that everything is taken care of for autoloading, and I don't need to do anything, when that isn't the case.

Regardless of the workflow, Composer Manager registers the autoloader generated by Composer so that modules can use the PHP classes contained in the libraries without having to explicitly require the class files.

cpliakas’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)

I understand the DX issues, but nobody is proposing any viable alternatives and answering the questions posed at #1992896-11: What recovery options do we have in case of RuntimeException.. If this wasn't a tricky problem then this module wouldn't need to exist. All I ask is that we really think through the "what if"s and propose alternatives instead of just saying the DX sucks. I totally agree that the DX isn't ideal, but I have put a lot of thought into mashing together how Drupal and Composer can work together in a way that is least disruptive to eachother's natural workflows. I am totally willing to accept (and actually assume) that I haven't formulated the best possible solution, but in order to move forward we really need viable alternatives that are well thought out and don't throw the baby out with the bath water.

With that being said, I as a maintainer haven't done a good job facilitating this issue because I am starting with the technical challenges which is a backwards approach. I opened #2007554: Discuss the use case and ideal DX for Composer Manager where we can discuss the ideal DX in absence of the technical issues and then work through the challenges from there. I am respectfully closing this issue as "won't fix" in order to drive discussions through that issue so we can start with the expected behavior and go from there.

Let me know if this makes sense,
Chris

cpliakas’s picture

Status: Closed (won't fix) » Active
cpliakas’s picture

As mentioned in #2007554: Discuss the use case and ideal DX for Composer Manager I am conceding on this issue. Let's register the autoloader automatically in order to improve the DX.

cpliakas’s picture

Category: feature » task
Priority: Normal » Major
Status: Active » Needs review
FileSize
477 bytes

Let's try a simple hook_init() approach and move forward from there. Happy to commit this and refine in later versions if it works and fits the more immediate need.

cpliakas’s picture

Title: Load autoload.php in hook_boot/hook_init? » Automatically register Composer's autoloader early in the bootstrap process to improve DX

Changing to title to remove implementation details in order to focus on the end goal.

RobLoach’s picture

I like it. Should something be reported on the Status Report page as well? Or do you think this is good scope for this issue?

cpliakas’s picture

Both the Status Report and Package Status pages already tell you what the state is and what needs to be done, e.g. do you need to generate a composer.json file? Run composer update? etc.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

:-)

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

pete-b’s picture

#17 patch worked nicely, thanks.

cpliakas’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

jbrown’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Closed (fixed) » Needs work

This needs to be fixed on Drupal 8.

It already has:

class ComposerManagerSubscriber implements EventSubscriberInterface {

  public function addAutoload(GetResponseEvent $event) {
    composer_manager_register_autoloader();
  }

  /**
   * {@inheritdoc}
   */
  static function getSubscribedEvents() {
    $events[KernelEvents::REQUEST][] = array('addAutoload');
    return $events;
  }
}

but I am not sure which event can be subscribed to for Drush.

cpliakas’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Needs work » Fixed

Let's move this to a separate thread so that we don't confuse topics. Re-marking this as fixed for the 7.x branch.

jbrown’s picture

Its the exact same issue - it should have been fixed first on D8 and then backported to D7.

If you close this issue, then you need to create a new one.

cpliakas’s picture

Since the discussion in this thread is about hook_init() and hook_boot(), it is not clear to me how this is the same issue. I kindly ask that you create another issue for D8 clarifying what you are asking and then we can work together to resolve.

Status: Fixed » Closed (fixed)

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

cpliakas’s picture