By throwing an exception when autoloaderer is not found is a little too harsh.

Why not watchdog with a 'WATCHDOG_EMERGENCY' which makes http://drupal.d7/admin/reports/dblog probably reachable in most cases.

CommentFileSizeAuthor
#10 composer-manager-1992896-10-1.png74.06 KBcpliakas
#10 composer-manager-1992896-10-2.png48.73 KBcpliakas
#10 composer-manager-1992896-10-3.png66.35 KBcpliakas
#10 composer-manager-1992896-10-4.png43.15 KBcpliakas
#9 composer-manager-recovery-option-1992896-9.patch1.22 KBclemens.tolboom
#1 composer-manager-no-wsod-1992896-1.patch642 bytesclemens.tolboom

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new composer-manager-no-wsod-1992896-1.patch642 bytes

The attached patch make my page related class loader failures with WSOD local :)

Even through I do

function graphapi_init() {
  composer_manager_register_autoloader();
cpliakas’s picture

cpliakas commented

Thanks for the patch, but I am not sure I agree.

I don't think that throwing an Exception is harsh, as the absence of an autoloader is an exceptional condition. If the autoloader is not available and you then try to instantiate a class, then you will get a fatal error that the class is not found and have to figure out why. Is it because the autoloader is not available? Did you mistype the classname? Is your namespacing wrong? Throwing an exception allows you to catch this specific error (and log the error via watchdog if you want) and proceed gracefully.

Also, be careful of loading the autoloader in hook_init(). The potential issues of doing this are discussed in detail at #1955386-2: Automatically register Composer's autoloader early in the bootstrap process to improve DX.

Let me know your thoughts on this,
Chris

robloach’s picture

robloach
he/him
commented

Maybe turrn it into something like:

function composer_manager_register_autoloader($throwException = true) {
}
cpliakas’s picture

cpliakas commented

I'd be in favor of that.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

@cpliakas the harshness if for module users UX when they enable the module and get a WSOD. The WSOD is even harsh for DX but yeah I agree.

By just logging the emergency message the can visit admin/config/reports probably to fix the problem which most of the times will have to do with running composer.

Regarding hook_init/hook_boot: I'm not sure when or even why I must call the composer_manager_register_autoloader :p

@RobLoach: by using #1 the UX is way better. It's recoverable depending on the other modules behaving according to plans.

Anyway the module helps much :)

robloach’s picture

robloach
he/him
commented

Agreed that having the call in hook_init()/hook_boot() would be much nicer as you wouldn't need to call composer_manager_register_autoloader() yourself. I'm in favour of that because then module's can assume that their dependencies are available. Unfortunately, modules can't assume that the Composer install has been run.

What I suggested in #3 is a compromise around that. Then at least you could do:

if (composer_manager_register_autoloader(false)) {
  $stuff = new MyStuff\MyStuff();
}
cpliakas’s picture

cpliakas commented

@clemens.tolboom,

I am still having a hard time seeing where this is a UX issue. Clearly a fatal error is no good, however the purpose of exceptions is to catch them and handle gracefully, not let them go uncaught so they cause a fatal error. I don't see why you can't try / catch composer_manager_register_autoloader() and set an error dsm, call watchdog, whatever to achieve the UX you want.

I want to make sure we aren't talking past each other, so I would be curious to hear your thoughts on this.

Thanks,
Chris

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Title: Prevent a WSOD by composer manager. » What recovery options do we have in case of RuntimeException.

Not reading any guide I just enabled 'Graph API' which requires Composer Manager. A good exception page appears but this is the end of a site not having shell access let alone run drush.

This is from using latest dev:

Error
Status message

    A composer.json file was written to /Users/clemens/Sites/drupal/d7/www/sites/default/files/composer.
    The configuration options have been saved.

Warning message
Composer's update command must be run to install the required packages.
Refer to the instructions on the Composer Manager project page for updating packages.
Error message
RuntimeException: Autoloader not found: /Users/clemens/Sites/drupal/d7/www/sites/all/libraries/composer/autoload.php in composer_manager_register_autoloader() (line 109 of /Users/clemens/Sites/drupal/d7/www/sites/default/modules/composer_manager/composer_manager.module).
The website encountered an unexpected error. Please try again later.

My guess is this will happen to the majority of casual Drupal users. They need module X which adds some required modules. These required modules are added. Module X is enabled and presto ... a dead site.

Can we prevent this 'disaster' by not enabling composer_manager at all through hook_requirements somehow?

Remember my module does:

function graphapi_init() {
  composer_manager_register_autoloader();
}

which is bad coding.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
StatusFileSize
new composer-manager-recovery-option-1992896-9.patch1.22 KB

Patch allows for a settings.php variable composer_manager_throw_exception which gives casual users a recovery options for this graphapi_init misbehaviour.

This is probably the best we can get.

IMHO graphapi_init should never call composer_manager_register_autoloader but that's probably #1989402: Chicken and egg: What is first, module or package?

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work
StatusFileSize
new composer-manager-1992896-10-4.png43.15 KB
new composer-manager-1992896-10-3.png66.35 KB
new composer-manager-1992896-10-2.png48.73 KB
new composer-manager-1992896-10-1.png74.06 KB

To start off, I definitely understand that this is a tricky issue and appreciate the effort that is being made to solve it. However, I am marking this as needs work because I think that the problem needs to be attacked in Graph API and not Composer Manager. Let's walk through some use cases and how the Monolog module handles them, because I think it shows how uncaught exceptions are not a product of the Composer Manager module but rather the implementing module.

Restating the problem

To restate the problem, based on how Drupal and Composer Manager work there will be some amount of time where the dependencies required by a module aren't available. To goal is to not throw fatal errors or uncaught exceptions, which is a clearly a bad experience for end users and site builders alike.

How do other modules handle the autoloader not being available?

Let's look at how Monolog handles this condition. To set up the scenario, take the following actions on a clean install of Drupal 7.

  • Manually download the latest releases of Composer Manager and Monolog (do not use Drush)
  • Enable Composer, Monolog, and Monolog Logging through the Drupal UI

At this point you have a condition where the the Monolog Logging module is handling watchdog() calls but the Monolog library is not available. The is one of the conditions that causes the problem we are trying to solve. However, notice that even if a message is logged through watchdog() by creating a node or something, no uncaught exceptions are thrown since Monolog correctly handles this.

Furthermore, if you visit admin/reports/status Composer Manager tells you what you need to do to pull in the libraries and provides a link to instructions on how to perform required actions.

composer-manager-1992896-10-1.png

If you visit the package status page at admin/config/system/composer-manager, you will see the same message, and it will also tell you which packages are required but not yet installed.

composer-manager-1992896-10-2.png

So at this point the application does not throw fatal errors and the site builder is hit over the head with what the state of the application is and what needs to be done to pull in the dependencies.

How do other modules handle the library not being available?

To set up the scenario, take the following actions on a clean install of Drupal 7.

  • Download the latest release of Composer Manager
  • Either enable Composer Manager with Drush, or enable Composer Manager via the UI and follow the instructions on Composer Manager's project page to run Composer's install command.
  • Visit admin/reports/status to make sure the autoloader is available. If so, proceed to the next step.
  • Manually download the latest releases of Composer Manager and Monolog (do not use Drush)
  • Enable Monolog and Monolog Logging through the Drupal UI

At this point you have the use case where the autoloader is available but the Monolog library has not yet been pulled in. Again, notice that even if a message is logged through watchdog() by creating a node or something, no uncaught exceptions are thrown since Monolog correctly handles this.

If you visit admin/reports/status, Composer Manager tells you which dependencies are missing and points the site builder to instructions on what needs to be done to pull them in.

composer-manager-1992896-10-3.png

Visiting the package status page at admin/config/system/composer-manager tells us the same.

composer-manager-1992896-10-4.png

Are there any other modules that work this way?

Yes. Although Monolog is highlighted above because it is an easy use case to replicate, the Git Wrapper module works the same way and doesn't throw uncaught exception or fatal errors.

What are the problems with the proposed approach?

Most importantly, I want to make sure that we have disproven the assumption that the only way to attack this problem is to change the behavior of Composer Manager. The use cases above show how there are developer / end-user friendly ways of handling the scenarios causing the problem we are trying to solve.

Second, this is an application level error, and consistency is super important for DX. Adding a magic setting that globally changes error handling behavior is not something we want to do. I want to know how a function behaves and not have to check a variable to handle it accordingly. By introducing this change we would have to re-code all implementing modules to handle the error differently based on whether this variable is set.

Third, if an exception is not thrown the function doesn't return TRUE or FALSE, so we have no idea whether it was successful. In our code we would have to perform a manual check anyways, which really defeats the purpose.

Where do we go from here?

I think we are focusing on a solution to solve a very specific use case. We really need to understand the reasons why the module behaves like it does and take into account how other modules would have to be modified to work with whatever change is proposed. I am happy to propose some techniques to Graph API to attack the root problem we are trying to solve, but we really have to ask "what if's" and explicitly highlight how any changes improve the developer / end user experience over the current implementation. Right now I don't feel like we have fully thought through these things.

Thanks,
Chris

cpliakas’s picture

cpliakas commented

The post above is long, but to restate what I am looking for I need the answers to the following questions when introducing changes like this:

  • Show with code how the proposed change improves DX over existing techniques.
  • How does the solution handle the use case where the autoloader is unavailable?
  • How does the solution handle the use case where the autoloader is available but the library is not?
  • How does the proposed change effect existing implementations?
  • Is the error handling technique consistent?
  • Does the developer / site builder have all the information the need to resolve the error condition? If so, where?

I am probably missing some items and will add them as they come up, but I hope this guideline is useful as we work together to make improvements.

Chris

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Thanks for the long answer. I'll check monolog next time.

Note my patch #9 is a UX recovery plan not a DX. I as the Graph API developer _can_ handle WSOD etc.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Just checked the monolog code. What it does is adding a wrapper around it's class.

function monolog($channel_name) {
  // We do not use drupal_static() since this is a high frequency function.
  static $loggers = array();

  if (!isset($loggers[$channel_name])) {
    try {

      composer_manager_register_autoloader();
      if (!class_exists('Drupal\Monolog\Logger')) {

That's not what I expected from a DX point of view. My module defines a dependency on composer_manager and defined a composer.json for further requirements thus should not be enabled if and when it's requirements a not fulfilled period.

So maybe we need just some developer documentation? (Or did I miss the docs?)

What about

function monolog_requirements($phase) {
  if ($phase == 'install') {
    try {

      composer_manager_register_autoloader();
      if (!class_exists('Drupal\Monolog\Logger')) {
        $requirements['drupal'] = array(
          'title' => $t('Monolog libraries not available'),
          'value' => VERSION,
          'severity' => REQUIREMENT_ERROR,
    ); 
...

Any way I stop commenting on this issue and try the above next week.

cpliakas’s picture

cpliakas commented

So maybe we need just some developer documentation? (Or did I miss the docs?)

No, there are no docs. I see no point in documenting things until the paradigms are set in stone. At this point I'd rather spend effort flushing out the use cases and doing what makes sense, and then documenting when we figure things out. IMHO documenting things too early cements things that should still be vetted.

Re: the requirements, that unfortunately won't work. Composer Manager only combines the requirements for modules that are enabled, so when you install the Monolog module (or any other module that has dependencies managed by Composer) the library won't be available yet. So in other words, the requirements will always fail during the install phase, and you would never be able to install modules that have Composer dependencies. That is why Composer Manager does it's check in the runtime phase.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

@cpliakas can you please review my patch #9 as I guess you misread it. It is a UX last resort recovery option just in case. I really hate WSOD for my users.

Regarding:

No, there are no docs

Agreed we need no bricks before all dust is settled. Maybe a hint on the project page to look into 'the other' projects?

Re: the requirements

Well that ... is not so nice ;)

From my DX point of view I should do one call somehow to instruct composer_manager instruct it to use my modules json through hook_requirements('install'). But that's another discussion I guess :)

cpliakas’s picture

cpliakas commented

Hi clemens.tolboom.

Check out the "What are the problems with the proposed approach?" in #10. As mentioned in #11, we really need to have an answer to all these questions before pulling anything in. I don't have any of these answers for the changes being proposed yet, therefore we cannot move forward.

Thanks,
Chris

cpliakas’s picture

cpliakas commented
Status: Needs work » Closed (works as designed)

So taking a step back... as a maintainer I am not doing a good job facilitating this issue since I am focusing on the technical details that are causing challenges and not the ideal experience. Therefore I am going to close this issue as "works as designed" and open a new issue at #2007554: Discuss the use case and ideal DX for Composer Manager where we can discuss the ideal DX in absence of the technical deals. Then we can figure out how to attack the barriers as opposed to starting with the technology which IMHO is backwards.

Thanks,
Chris

cpliakas’s picture

cpliakas commented
Issue summary: View changes

Fixed typo

RonP’s picture

RonP commented
Issue summary: View changes

Jesus age christ. You guys know about this ? And it is still going on ? Irresponsible is all I have to say

My site is dead. And this is as far as I have gotten searching for a cure.

If this isn't a joke please tell me where it doesn't make me cry.