Currently, the autoloader is only registered when a request happens.

I have an install profile that includes composer_manager and search_api_solr, which needs a composer package, for example in hook_cron(), which is called during the installer.

If you install with drush, then everything happens in the same process, so there is never a new request that would then register the autoloader before that runs.

Similar things could happen if you install multiple modules together in the UI that depend on packages being there.

Suggestion: Register in hook_install().

CommentFileSizeAuthor
#5 composer_manager-install-2345285-5.patch411 bytesberdir
#1 composer_manager-install-2345285-1.patch806 bytesberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new composer_manager-install-2345285-1.patch806 bytes

Here is a patch, replacing the update function.

cpliakas’s picture

cpliakas commented
Priority: Normal » Major

Thanks for the contribution, reasoning seems solid to me.

One question I have is why this is an either / or with the update hook. The update hook exists to aide in D7 -> D8 upgrades, and I am unclear why it is interfering with the fix you proposed.

Thanks!
Chris

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

It is completely unrelated to the update function. But 7.x -> 8.x update is no longer supported and the function used there no longer exists, it needs to be a migration now. Feel free to keep the function as a reminder for defining a migration if you want to (maybe add a @todo then).

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work

Thanks for the clarification, definitely makes sense to remove the update function.

Sorry to be a pain about this, but could we separate out removing hook_update_N() into a separate issue and patch? I created an issue at #2346843: Remove the composer_manager_update_8000() function referencing your comment, happy to commit as soon as a separate patch is rolled. I am not that smart, so it helps me to separate things out into their own threads so I can follow what is going on and not get confused later when trying to remember why things were done :-)

If we can re-roll the patch in this thread to only include the hook_install() function, I will RTBC and commit ASAP.

Thanks again for your contribution(s),
Chris

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new composer_manager-install-2345285-5.patch411 bytes

Updated the patch.

bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Committed, thank you.

  • bojanz committed bcdac1d on 8.x-1.x authored by Berdir 
    Issue #2345285 by Berdir: Autoloader not a registered during...

Status: Fixed » Closed (fixed)

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

bojanz’s picture

bojanz commented
Status: Closed (fixed) » Needs work

Reverted, it was causing #2390675: autoload.php doesn't exist anymore.

I'll need to look into this more.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Uh, right, this only worked for me because the files were already there.

bojanz’s picture

bojanz commented
Status: Needs work » Closed (duplicate)

This will be resolved by #2350639: Rewrite 8.x-1.x, composer_manager will stop having its own autoloader.