Install a module user interface does not install modules (or themes)

Still the case with 8.0-alpha2+732-dev of 28 Aug.

Tested on Devel module (http://ftp.drupal.org/files/projects/devel-8.x-1.x-dev.zip of 14 Aug) which is about the only version 8 module I can find which (mostly) works! and Red theme (http://ftp.drupal.org/files/projects/red-8.x-1.0-alpha2.zip of 25 July).

Tried this again today with the latest HEAD - no change. Tried Devel module, both the URL and downloaded zip file. As the OP says, the page just refreshes with the URL empty or the Browse showing "No file selected". No errors reported, but nothing happens.

I would have thought this is critical 'cause it is a major regression from Drupal 7 and we could not release anything with this functionality broken!

I partially traced this process and it seems to create a batch properly. I think it dies somewhere in core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/RedirectResponse.php -- a Symfony2 file.

The url parameter it's trying to pass is the batch process url, I believe: http://example.com/core/authorize.php?batch=1
Manually going there in browser results in a white screen even with error reporting on.

Logged this one earlier as https://drupal.org/node/2071117

I am still seeing the same issues as I reported and this 'core' bug is still 'unassigned' - what can i do to help? being a newbie bites, i can test if there's a new release that's had a fix applied..just let me know!

git-bisected to this commit. and its issue: #1788888: Overlay at admin/modules closes on submitting the form.

So can you try the same without the overlay?

Installed Drupal 8 latest HEAD using English and Standard profile.

Attempted to uninstall Overlay module. This did not go smoothly! I will post separate bug report.

Once I had things working again (and had checked that Overlay module was unticked on the Extend screen and I was not seeing overlay in admin UI), I attempted to install Devel module both using the URL and from a downloaded zip file.

No difference. The page still refreshes with the URL and file fields empty. No error messages. No files copied.

@aspilicious
I tried this on HEAD.
from the overlay, the problem still exists.
from the path admin/modules/install, it works right, and is redirected to core/authorize.php

Since there's a workaround (uninstalling overlay), downgrading. Also this might want to be postponed on #1889150: Simplify overlay look, *only use for contextual operations* or #2088121: Remove Overlay.

I have disabled the use of Overlay in the user edit page, then I tried to install a module, and I was redirected to authorize.php. I think this the desired action.

Regards,
K.

Updated OP with steps to reproduce

Overlay is dead to D8 #2088121: Remove Overlay.

I reported in #7 that removing the overlay module made no difference. Now that overlay module has gone, I tested it again. It still behaves exactly as reported in OP - the page refreshes with the URL and file fields empty. No error messages. No files downloaded or copied.

updated the steps to reproduce.
for a direct upload, (not going through ftp, etc), this condition should be met:

  // If the owner of the directory we extracted is the same as the
  // owner of our configuration directory (e.g. sites/default) where we're
  // trying to install the code, there's no need to prompt for FTP/SSH
  // credentials. Instead, we instantiate a Drupal\Core\FileTransfer\Local and
  // invoke update_authorize_run_install() directly.
  if (fileowner($project_real_location) == fileowner(conf_path())) {
    module_load_include('inc', 'update', 'update.authorize');
    $filetransfer = new Local(DRUPAL_ROOT);
    call_user_func_array('update_authorize_run_install', array_merge(array($filetransfer), $arguments));
  }

so, to reproduce this bug, make sure your sites/default owner is same as your web server's owner, e.g. www-data.

After some debugging, I have found a chain of function calls in update_manager_install_form_submit (update.manager.inc), which in the end, throws a RedirectResponse away:

function update_manager_install_form_submit($form, &$form_state):
  call_user_func_array('update_authorize_run_install', array_merge(array($filetransfer), $arguments));
  -> return system_authorized_batch_process();
    ->  return batch_process($finish_url, $process_url);
      ->  return new RedirectResponse(url($batch['url'], $options));

git blaming that new RedirectResponse() leads to this change in #1668866: Replace drupal_goto() with RedirectResponse:

-      $function = $batch['redirect_callback'];
-      $function($batch['url'], array('query' => array('op' => 'start', 'id' => $batch['id'])));
+      $options = array('query' => array('op' => 'start', 'id' => $batch['id']));
+      if (($function = $batch['redirect_callback']) && function_exists($function)) {
+        $function($batch['url'], $options);
+      }
+      else {
+        $options['absolute'] = TRUE;
+        return new RedirectResponse(url($batch['url'], $options));
+      }

How to actually solve this issue isn't clear to me.
should we add a value to $batch['redirect_callback']? (which is NULL here) or use the returned RedirectResponse in a way like below? (which manages to install a module, but fails in so many other ways that prevented me from posting as a real patch)

diff --git a/core/modules/update/update.manager.inc b/core/modules/update/update.manager.inc
index 19c0dc1..2735cda 100644
--- a/core/modules/update/update.manager.inc
+++ b/core/modules/update/update.manager.inc
@@ -751,7 +751,12 @@ function update_manager_install_form_submit($form, &$form_state) {
   if (fileowner($project_real_location) == fileowner(conf_path())) {
     module_load_include('inc', 'update', 'update.authorize');
     $filetransfer = new Local(DRUPAL_ROOT);
-    call_user_func_array('update_authorize_run_install', array_merge(array($filetransfer), $arguments));
+    $response = call_user_func_array('update_authorize_run_install', array_merge(array($filetransfer), $arguments));
+    // Save $_SESSION data from batch.
+    drupal_session_commit();
+    // Send the response.
+    $response->sendHeaders();
+    exit;
   }
   // Otherwise, go through the regular workflow to prompt for FTP/SSH
   // credentials and invoke update_authorize_run_install() indirectly with

Any guides on how to move this issue forward would be appreciated.

@herom, this corroborates what I reported in Comment #3 above. Like you, I was somewhat perplexed where to go from there. I'm glad to see that we seem to be building a consensus on the cause now.

Linking with #842620: Update manager can't install modules using FTP due broken FileTransferAuthorizeForm. They are not quite duplicates, as that one is looking for problems other than file permissions.

Still a problem on alpha11 - fresh install - tried to install backup_migrate so I can save an image that is easy to go back to.

#2250613: Installing module doesn't work seems like another dupe.

Still a problem on alpha13

Patch attached. Needed to test #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig

I noticed the same problem would happen with updates

Re-roll, thanks a bunch @SebCorbin! That helped make this work.

Not sure if I'm missing something. Patch in #27 alter form_state with a methode, only that it is not a object. Fail horrible on my system with:

PHP Fatal error:  Call to a member function setRedirectUrl() on a non-object in /drupal/core/modules/update/src/Form/UpdateManagerInstall.php on line 211, referer: http://d8.dev/admin/modules/install

PHP Stack trace:, referer: http://d8.dev/admin/modules/install
PHP   1. {main}() /drupal/index.php:0, referer: http://d8.dev/admin/modules/install
PHP   2. Drupal\\Core\\DrupalKernel->handle() /drupal/index.php:23, referer: http://d8.dev/admin/modules/install
PHP   3. Symfony\\Component\\HttpKernel\\HttpKernel->handle() /drupal/core/lib/Drupal/Core/DrupalKernel.php:563, referer: http://d8.dev/admin/modules/install
PHP   4. Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw() /drupal/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:66, referer: http://d8.dev/admin/modules/install
PHP   5. call_user_func_array:{/drupal/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:126}() /drupal/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:126, referer: http://d8.dev/admin/modules/install
PHP   6. Drupal\\Core\\Controller\\HtmlPageController->content() /drupal/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:126, referer: http://d8.dev/admin/modules/install
PHP   7. Drupal\\Core\\Controller\\HtmlPageController->getContentResult() /drupal/core/lib/Drupal/Core/Controller/HtmlPageController.php:53, referer: http://d8.dev/admin/modules/install
PHP   8. call_user_func_array:{/drupal/core/lib/Drupal/Core/Controller/HtmlPageController.php:76}() /drupal/core/lib/Drupal/Core/Controller/HtmlPageController.php:76, referer: http://d8.dev/admin/modules/install
PHP   9. Drupal\\Core\\Controller\\FormController->getContentResult() /drupal/core/lib/Drupal/Core/Controller/HtmlPageController.php:76, referer: http://d8.dev/admin/modules/install
PHP  10. Drupal\\Core\\Form\\FormBuilder->buildForm() /drupal/core/lib/Drupal/Core/Controller/FormController.php:80, referer: http://d8.dev/admin/modules/install
PHP  11. Drupal\\Core\\Form\\FormBuilder->processForm() /drupal/core/lib/Drupal/Core/Form/FormBuilder.php:249, referer: http://d8.dev/admin/modules/install
PHP  12. Drupal\\Core\\Form\\FormSubmitter->doSubmitForm() /drupal/core/lib/Drupal/Core/Form/FormBuilder.php:566, referer: http://d8.dev/admin/modules/install
PHP  13. Drupal\\Core\\Form\\FormSubmitter->executeSubmitHandlers() /drupal/core/lib/Drupal/Core/Form/FormSubmitter.php:56, referer: http://d8.dev/admin/modules/install
PHP  14. call_user_func_array:{/drupal/core/lib/Drupal/Core/Form/FormSubmitter.php:132}() /drupal/core/lib/Drupal/Core/Form/FormSubmitter.php:132, referer: http://d8.dev/admin/modules/install
PHP  15. Drupal\\update\\Form\\UpdateManagerInstall->submitForm() /drupal/core/lib/Drupal/Core/Form/FormSubmitter.php:132, referer: http://d8.dev/admin/modules/install

Altered this back to "good" old arrays. It copied/decompressed the module though still need some work.

+++ b/core/modules/update/src/Form/UpdateManagerInstall.php
+++ b/core/modules/update/src/Form/UpdateManagerInstall.php
@@ -208,7 +208,7 @@ public function submitForm(array &$form, array &$form_state) {

@@ -208,7 +208,7 @@ public function submitForm(array &$form, array &$form_state) {
-      $form_state->setRedirectUrl(system_authorized_batch_processing_url());
+      $form_state['redirect'] = system_authorized_batch_processing_url();

+++ b/core/modules/update/src/Form/UpdateReady.php
+++ b/core/modules/update/src/Form/UpdateReady.php
@@ -134,7 +134,7 @@ public function submitForm(array &$form, array &$form_state) {

@@ -134,7 +134,7 @@ public function submitForm(array &$form, array &$form_state) {
-        $form_state->setRedirectUrl(system_authorized_batch_processing_url());
+        $form_state['setRedirectUrl'] = system_authorized_batch_processing_url();

That is weird those form_state variables are not arrays in my repo. Are you in 8.0.x branch? or 8.x?

Also typo 'setRedirectUrl' key should be 'redirect' key like the first one.

Ehem.... git branch -D 8.x —and, moving on :)

#27 Installs, though one notice:
Notice: Array to string conversion in Drupal\Component\Utility\SafeMarkup::set() (line 77 of core/lib/Drupal/Component/Utility/SafeMarkup.php).

Drupal 8.0.x HEAD
Install profile minimum

As I quick notice, there is an oddity getting to the module installer. After drupal is installed.

See? The link is missing. To fix the problem run: 'drush cache-rebuild'. If you omit running cache rebuild going to these:

admin/modules/install

Type  php
Date  Monday, September 1, 2014 - 21:53
User  admin
Location  http://d8.dev/admin/modules/install
Referrer  
Message   InvalidArgumentException: No check has been registered for access_check.update.manager_access in Drupal\Core\Access\AccessManager->loadCheck() (line 343 of /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Access/AccessManager.php).

http://d8.dev/admin/reports/updates/settings

Type  php
Date  Monday, September 1, 2014 - 21:53
User  admin
Location  http://d8.dev/admin/reports/updates/settings
Referrer  http://d8.dev/admin/modules
Message   InvalidArgumentException: Class "\Drupal\update\UpdateSettingsForm" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (line 29 of /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php).

Is update installed?

drush pml | grep update
Core  Update Manager (update)  Module  Enabled  8.0.0-dev

So claim to be installed, though running:

drush en update --yes
update is already enabled.                                                                          [ok]
The following extensions will be enabled: update, file
Do you really want to continue? (y/n): y
file was enabled successfully.                                                                      [ok]
update was enabled successfully.

Does something. Going to /admin/modules now break the entire site. Running 'drush cache-rebuild' fixes the problem.

Standard install profile is pretty much the same

No Update link and:

drush en update --yes
update is already enabled.                       [ok]
The following extensions will be enabled: update
Do you really want to continue? (y/n): y
update was enabled successfully.                 [ok]

claim to install update, not file as with minimum install profile. Going to http://d8.dev/admin/modules

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "update.module_install" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 147 of /Users/steinmb/sites/d8/drupal/core/lib/Drupal/Core/Routing/RouteProvider.php).

This kinda works, but the redirect is done. Hiding #28

Please review and patch based on #27

Added issue summary template and filled out the best I could, but the resolution should be refined, along with the remaining tasks.

Just tried patch 27. It is fixing the issue however update is forcing my http connection to https and then the request stops because my server does not have ssl setup. Is this normal for update manager to switch to https from http?

I installed the beta today and wanted to install a module via interface. The request switched to https as tsikerdekis reports and i get 'not authorized' page. This after I enabled ssl on my ubuntu/apache. I uncomment ...authorize... in settings.php and retry. Now it doesn't switch to https but I still get 'not authorized'.

We'll keep the version at -dev because that is where this patch applies that is trying to fix the issue. The patch makes some progress but doesn't solve the issue. Someone many need to dig deep to sort this one out further, though applying and testing out the patch is a good jumping off point.

Somebody also showed this problem at DrupalCamp Ghent on Thursday. He was very startled when he noticed that this feature which he used all the time didn't work at all.

So based on #38, it sounds like there is an additional regression in Drupal 8 where the functionality is more broken? In Drupal 7, this was a bad usability problem, but it "worked" (in a manner of speaking).

@catch's assessment in #2352637: Remove the UI for installing/updating modules from update module if it is not fixed in time for release is that it is critical to either fix or remove this feature. However, for this issue to be critical on its own, that would mean the other issue would be duplicate. So I think this issue should be major, and we try to resolve it during the beta, but in case it does not go in, then we do #2352637: Remove the UI for installing/updating modules from update module if it is not fixed in time for release instead?

I object to removing this functionality. I know I should use drush, but I don't, so this is absolutely critical to me. I spend a lot of time playing with contrib modules and seeing what works and what fails and report bugs, so if you don't make it work you lose me.

I use this feature extensively on D7 and have never seen any problems with it.

I used the latest patch from #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig to help test the UI but it's not necessary for this and is actually a blocker for that patch.

The SSL was causing me a problem so I hard coded the URL instead of using that system_authorized_get_url() as previous patches.

This works though, please test it.

I realize the audience who would use this feature may not be familiar with the command line but I'd really like their feedback and testing as well.

The quickest way to try this patch out without git even is with this command in your drupal 8 root folder.

curl https://www.drupal.org/files/issues/install_a_module_user-2042447-40.patch | patch -p1

Hope that helps, and you may have to clear cache after applying that patch and hopefully it applies for you all!

+++ b/core/modules/update/src/Form/UpdateReady.php
@@ -141,8 +142,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
       // whatever FileTransfer object authorize.php creates for us.
       else {
         system_authorized_init('update_authorize_run_update', drupal_get_path('module', 'update') . '/update.authorize.inc', array($updates), $this->t('Update manager'));
-        $form_state->setRedirectUrl(system_authorized_get_url());
       }
+      $redirect_url = Url::fromUri('base://core/authorize.php');
+      $form_state->setRedirectUrl($redirect_url);
     }

Why is this moved out of the if/else? Won't this redirect to authorize.php in cases where it isn't needed? Also in the next hunk.

I had looked into this in #2311691: Updater UI for installation is broken, and it is a baffling problem. I just wonder if this patch treats some of the surface symptoms, or solves the real underlying problem?

It's not redirecting after the submit to the authorize page, this solves that problem and that is why it's out side the condition. This may not be absolute solution but it's a path forward.

Also the existing function call to generate that URL was forcing https which also was a problem.

The cleanup on fail of the temporary dir can compound the problem but I didn't attempt to solve that issue relating to the archiver with leftover files in /tmp

Of course all reviews welcome, just trying to move this forward.

Please give suggestions or an alternative patch if you think my approach is incorrect. I'll gladly test it

I think the general problem is that `system_authorized_get_url()` forces HTTPS regardless if it is available or not, and that the UnroutedUrlAssembler doesn't do mixed mode detection.

Maybe something like the attached (which is totally untested)?

So these patches behave differently if you run your site fully in SSL. right? It seems that the modern trend is to do so for all websites and perhaps Drupal should join the crowd. And yes, that means sites have to purchase a certificate, which is admittedly not necessary for a simple brochure site with no users. But uid=1 is a user and if s/he logs on from Starbucks, the site might be compromised.

Pretty sure I have confirmation that #2373445: UnroutedUrlAssembler() doesn't check whether mixed mode sessions are enabled is a bug, which would likely be cause of the problems here.

And the layers on the onion start to come off.

Still present in Beta 3 (no patches tried).

Upload via URL

On entering a URL to a module archive (both gzip and zip), and clicking Intall, the file is written to the screen (zip shows "PK" in top-left corner). After finishing the display, the Install screen shows the message "Provided archive contains no files".

The logs show 3 errors:

Notice: Undefined offset: 0 in update_manager_archive_extract() (line 164 of /path/drupal8/core/modules/update/update.manager.inc).

Notice: Use of undefined constant CURLOPT_TIMEOUT_MS - assumed 'CURLOPT_TIMEOUT_MS' in GuzzleHttp\Ring\Client\CurlFactory->applyHandlerOptions() (line 402 of /path/drupal8/core/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php).

Warning: curl_setopt_array(): Array keys must be CURLOPT constants or equivalent integer values in GuzzleHttp\Ring\Client\CurlFactory->__invoke() (line 52 of /path/drupal8/core/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php).

I do not have curl installed. I presume /tmp is writable (not error message), though it is not yet protected.

Upload by Browsing local file

In "Upload a module or theme archive to install", I click Browse, select a local file, then Install. I get returned to the Install screen, with no error message, and no log entry. Additionally, a zip file blanked the screen, gzip just returned to the screen.

@iantresman, can you try the patch and let us know what happens. We need more people testing this to give us clues as to what the root problem is.

@mpdonadio. I think I patched the three files OK, which I had to do manually as I don't use an IDE, and I couldn't get Eclipse, Netbeans, phpStorm demo, or Aptana Studio to patch successfully (astonishing there isn't a standalone utility which lets you apply a patch file to a specified file).

1. Install new module (URL, GZIP)

   I now have a new error message. After entering a URL to a gzip file, and clicking install, I get the error (There is no new logged error.):

   Archivers can only operate on local files: temporary://update-cache-1239c21d/google_analytics-8.x-2.x-dev.tar.gz not supported

2. Install new module (Upload, GZIP)

   If I enter a URL to a zip file, then the file is written to the screen.

3. Install new module (URL, GZIP)

   On Browsing a GZIP file locally on my PC, I get the blue uploading fuel gauge, and I end up at the install page with no messages. But the module successfully appears on the module extend page (and can be enabled). But my Drupal logs show two new messages:

   Notice: Use of undefined constant CURLOPT_TIMEOUT_MS - assumed 'CURLOPT_TIMEOUT_MS' in GuzzleHttp\Ring\Client\CurlFactory->applyHandlerOptions() (line 402 of /path/drupal8/core/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php).

   Warning: curl_setopt_array(): Array keys must be CURLOPT constants or equivalent integer values in GuzzleHttp\Ring\Client\CurlFactory->__invoke() (line 52 of /pathdrupal8/core/vendor/guzzlehttp/ringphp/src/Client/CurlFactory.php).

   On enabling the uploaded module, a Drupal log entry confirms that the module was installed (I think it should say that the module was enabled. But there is no log entry to say that the module was successfully uploaded.

4. Install new module (Upload, ZIP)

   On Browsing a zip file locally on my PC, and clicking install, I get a blank page.

Simillar to mentioned in #30 when applying path from @joelpittet I get the following :

Drupal 8

Update manager

Status message Installation was completed successfully.
Error messageNotice: Array to string conversion in Drupal\Component\Utility\SafeMarkup::set() (line 77 of core/lib/Drupal/Component/Utility/SafeMarkup.php).

@joaogarin I believe that error is because this issue's fix isn't in yet:
#1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig

Marked #2362139: Installing mod/theme problem as a duplicate of this issue.

@joelpittet

Dear joelpittet,

Thank you very much for your simply one line patch that even I was able to apply on beta3. It works!!

However, for one theme I installed I got this error:

Authorize file system changes

Status message Installation was completed successfully.
Error messageNotice: Array to string conversion in Drupal\Component\Utility\SafeMarkup::set() (line 77 of core/lib/Drupal/Component/Utility/SafeMarkup.php).
ombak

Array

As shown in comment number 30

Thanks again

@aardwolfx you're welcome, seems we are making some progress.

I also think that is related to the issue I mentioned in comment #53 Which we still have to fix:)

+++ b/core/modules/update/src/Form/UpdateManagerInstall.php
@@ -218,7 +217,11 @@
+    if (!\Settings::get('mixed_mode_sessions', FALSE)) {

+++ b/core/modules/update/src/Form/UpdateReady.php
@@ -143,7 +142,11 @@
+      if (!\Settings::get('mixed_mode_sessions', FALSE)) {

Settings class doesn't exist in these files...

Ok so they fixed the SSL stuff since the last patch so this is a bit simpler.

I'm not sure about the batch process change in here but it feels like it's special cased and really don't need to be.
Maybe someone with more experience on the batch api can explain why it needs to be different?

Meanwhile this patch also deletes the two unused functions.

Here's an overview of the patch:

1. +++ b/core/modules/system/system.module
   @@ -418,7 +418,6 @@ function system_authorized_init($callback, $file, $arguments = array(), $page_ti
   -  global $base_url;
   
   @@ -427,19 +426,6 @@ function system_authorized_get_url(array $options = array()) {
   -function system_authorized_batch_processing_url(array $options = array()) {
   
   @@ -450,17 +436,6 @@ function system_authorized_run($callback, $file, $arguments = array(), $page_tit
   -function system_authorized_batch_process() {
   

   Unused functions and variables.

2. +++ b/core/modules/update/src/Form/UpdateManagerInstall.php
   @@ -227,8 +227,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -      $form_state->setRedirectUrl(system_authorized_get_url());
   ...
   +    $form_state->setRedirectUrl(system_authorized_get_url());
   
   +++ b/core/modules/update/src/Form/UpdateReady.php
   @@ -152,8 +152,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -        $form_state->setRedirectUrl(system_authorized_get_url());
   ...
   +      $form_state->setRedirectUrl(system_authorized_get_url());
   

   Redirect URL set regardless.

3. +++ b/core/modules/update/update.authorize.inc
   @@ -51,9 +51,8 @@ function update_authorize_run_update($filetransfer, $projects) {
      batch_set($batch);
   ...
   -  return system_authorized_batch_process();
   
   @@ -95,10 +94,9 @@ function update_authorize_run_install($filetransfer, $project, $updater_name, $l
   -  return system_authorized_batch_process();
   ...
   +  batch_set($batch);
   

   Let the batch_set() deal with the batch, the system_authorized_patch_process is doing some strange special case.

I have applied the #58 patch on latest drupal dev version (windows machine) and gave me error.

1. Enabled update manager module
2. Clicked Install new module
3. Uploaded a zip file
4. Module placed a drupal/modules folder i have created a folder contrib previously, so i think it should go to 'drupal/modules/contrib'.
5. After form submit got error please see attached screenshot.

Thanks,
Vj

@Vj does that happen without the patch?

The location it saves to may be a different issue. I'm taking this one problem at a time;)

yes i have applied the patch and uploaded zip file. Module saved at "Drupal/modules" folder but gave error.

Sorry, I'm just curious how it's different from without the patch applied on your machine.

Without patch just a page reloaded on form submit nothing happened same as original issue.

@Vj ok cool, that bug may have been exposed but I don't think it's related to this issue, nevertheless, I've created a new issue for you with a patch that may help in that regard. Though, to test it you will need this patch too.

Please try the patch there to see if it resolves that issue.
#2414399: Fatal error when installing module/theme

Don't want to include that fix in this issue because I don't run into that error, but it looks like it's either a namespace error or you may have an old version of PHP. Please check you have PHP > 5.4.5 @see https://www.drupal.org/requirements

Really need @yched to review the batch API stuff in #58 because it does make the install form actually function correctly but it seems to be the only form that tries to call batch_process() directly.

@yched Ping me on IRC if you have any questions but I think you are my only hope here.

Disclaimer :
- I didn't closely follow the batch API refactors that were made in D8 for routing and HttpKernel :-/
- I'm very unfamiliar with update.module and authorize.php, and am slowly wrapping my head around it (trying to compare D7, D8 HEAD and D8+patch)

Thus, summarizing my understanding of the issue below, please forgive me if this is just a verbose repetition of some of the discussion above, and correct me if I'm missing something :-)

The crux of the issue seems to be that, unlike in D7, batch_process() no longer ends execution by drupal_goto() right away, but returns a RedirectResponse instead. If the caller doesn't grab the return value and transfers it upwards as the response for the request, the redirect is lost and the batch is never executed.

If I get things right, what happens in HEAD is:

- update_authorize_run_XXX() are used to set up "batching through authorize.php as a progress page", calling system_authorized_batch_process() / batch_process()

- The call chain correctly transfers batch_process()'s RedirectResponse back as the return value of update_authorize_run_XXX(),

- BUT: the submitForm() callers of update_authorize_run_XXX() ignore that return value:
If the target directory is writable, update.module forms (UpdateReady and UpdateManagerInstall's submitForm()) run update_authorize_run_XXX() directly and ditch the return value --> no redirect.
if the target directory is not writable, those forms redirect to authorize.php / FileTransferAuthorizeForm, registering 'update_authorize_run_XXX' for delayed execution after authorization UI steps have been performed. FileTransferAuthorizeForm is the one that does the call, and it ditches the result as well --> no redirect either

- Form API (Drupal\Core\Form\FormSubmitter::doSubmitForm()) usually catches the batches that were set during submitForm(), and handles calling batch_process() and doing the redirect. But since batch_process() has already run, it just ignores those batches.
--> no redirect either

Then, trying to think of the correct fix:

- update_authorize_run_XXX() is in charge of setting up the batch for the operation at hand,
- It is always called in the formSubmit() of a form that wants to trigger the batch - either UpdateReady/UpdateManagerInstall or FileTransferAuthorizeForm if authorization UI steps are needed.
- The recommended way to trigger a batch in a form submit is to just do batch_set($my_batch) and let FAPI do the batch_process() in Drupal\Core\Form\FormSubmitter::doSubmitForm()
- Thus, the current patch feels right IMO:
update_authorize_run_XXX() should just do their batch_set(), instead of currently doing a batch_process() as well. For clarity, they should be renamed update_authorize_set_XXX_batch() though. Also, no need for them to return anything, and no need for their callers to do anything with the return value.

However, I might be missing something, but the intent of system_authorized_batch_process() in D7 and HEAD was to use authorize.php as the progress page (and authorize.php has dedicated batch logic code to display batch pages). It looks like the current patch, by deferring to FormSubmitter::doSubmitForm() to handle the batch_process(), will use the regular '/batch' URL (system.batch_page.html route) ?

Not sure about the exact implications here, but
- if we want to keep authorize.php as the progress page, the $batch arrays defined by update_authorize_set_XXX_batch() should include the following to reproduce what system_authorized_batch_process() was doing :

$batch['url'] = system_authorized_batch_processing_url();
$batch['batch_redirect'] = system_authorized_get_url()->toString();

- If we don't use authorize.php as the progress page, then we have some cleanup to do in authorize.php ?

if we want to keep authorize.php as the progress page, the $batch arrays defined by update_authorize_set_XXX_batch() should include [some code] to reproduce what system_authorized_batch_process() was doing

That could be the job of a repurposed version of system_authorized_batch_process(), that adjusts special urls in the $batch instead of calling batch_process($special_urls) ?

function system_authorized_batch($batch) {
  $batch['url'] = system_authorized_get_url(['query' => ['batch' => '1']]);
  $batch['batch_redirect'] = system_authorized_get_url()->toString(); 
  return $batch;
}

Oh, also : patch needs a reroll ;-)

@yched thanks so much gives us a bunch of things to try out and the history. I'll try a few more approaches and see that the redirectResponse doesn't get lost.

@joelpittet : no problem - Batch API internals are not exactly friendly :-/ I wish I had the bandwidth to at least modernize it a bit in the D8 cycle, but well, other stuff happened...

I'll try a few more approaches and see that the redirectResponse doesn't get lost.

To be clear, I think the approach in the current patch is correct : just do a batch_set() within the submitForm() handlers, let Form API take care of doing the batch_process() and handling its RedirectResponse return value.

The end of #67 points a couple adjustments that I think would be needed for that approach but I think we're on the right track here.

@yched, thanks for clarifying, the part I'm most confused about is the talk about update_authorize_run_XXX() as that is not something I've seen while dealing with this patch so it doesn't fit into my mental model of how things work.

@yched seems that 'batch_redirect' key doesn't get picked up unless it's passed through the form_state object.

batch.inc

...
 if ($_batch['form_state']->getRedirect() === NULL) {
      $redirect = $_batch['batch_redirect'] ?: $_batch['source_url'];
      // Any path with a scheme does not correspond to a route.
...

So I'm guessing I need to set the redirect on the the FormState object as well...

Which I guess verifies the move I made of

    $form_state->setRedirectUrl(system_authorized_get_url());

for both conditions.

Here's a re-roll/fixing up things.

I dropped the functions that really just help obscure what's going on and assigned their value directly to the batch definition.

The $batch['redirect_url'] seems to be working of the form's redirect. I'm thinking the 'url' is going to batch.php though, so I'm working on debugging that, but I think this is at least a working patch.

Doing batch_set(); with a 'url' in this case adds it to a $batch['sets'][0] somewhere in the call stack, which may be why they are calling batch_process() directly to set the URLs.

Hmm same difference... doing the batch_set() right on the submitHandler. Both approaches kinda work but the url it's going to is batch.php?batch=1 and not core/authorize.php?batch=1

This is what you were getting at @yched?

Ok the problem with this approach is when you do batch_set(), it's values are stored in a $batch_set, not on the $batch... this makes things a bit confusing. batch_process() from the form submit handler doesn't take any arguments and that's the only way to override $process_info's 'url' and 'batch_redirect'.

Sooo unless we change how the batch_process() in FormSubmitter to take in some arguments, we need to call batch_process() directly from the submit Handler and deal with the Response it seems.

Ok this needs some review/testing BUT it solves:

1. Batch url/redirect not getting set.
2. The progress bar failing on the authorize.php page because the JsonResponse wasn't dealt with.
3. A double [[array]] happening from _batch_page().

May still need a docblock/function name fix for system_authorized_batch_process()

Please test this!

FYI, if you get an FTP page, this is somewhat of a separate issue, it's a permissions issue that I'll likely need to make a follow up issue for, or try to resolve it here too...

Oh, right, my bad :-(
The URLs for batch processing (progress page, final redirect), that are assigned by batch_process($redirect_url, $progress_url), do not live at the same level than the array passed to batch_set(), and thus cannot be pre-assigned there. Sorry for the wrong pointers...

Mulling on that a bit.

+++ b/core/modules/update/update.authorize.inc
@@ -50,8 +50,8 @@ function update_authorize_run_update($filetransfer, $projects) {
-
   batch_set($batch);
+

Needless hunk ? ;-)

Rerolled, I kept the "needless hunk" from #81 as it is more readable, and removed the return as system_authorized_batch_process() returns void.

The finished process redirected to "/core/authorize.php/<none>", solved that by not using ->toString (which later translates as '<current>', producing '/<none>' as it a unrouted url) but using the same url without 'core/' (as it will be our basePath when batch is finished).

I really don't know if is the right way, but at least we end up on the right url.

+++ b/core/modules/system/system.module
@@ -414,34 +414,28 @@ function system_authorized_init($callback, $file, $arguments = array(), $page_ti
+ *   Optional batch redirect flag, use if the page need to redirect on itself
+ *   after a batch is finished.

Minor grammar: "use if the page needS to redirect on itself"

--

I made some updates to the issue summary, the part that really needs an update is the Proposed resolution and I don't understand the patch enough to do that.

Without the patch, the behaviour described in the issue summary is what I'm seeing. You are sent back to an empty form, unless you use the URL option the first time and get:

Archivers can only operate on local files: temporary://update-cache-eff7ed19/devel-8.x-1.x-dev.tar.gz not supported

I tested #82 several times with both an URL (http://ftp.drupal.org/files/projects/devel-8.x-1.x-dev.tar.gz) and local uploads of that file. With the URL I sometimes saw the Archiver error pointed out in #44 and #51. It seems like it goes away the second time you try the URL, and I'm not sure how to bring it back other than reinstalling. Deleting the files/folders from /tmp didn't seem to do it.

The 'Archivers' error seems like it could be taken up on a separate issue, the patch here certainly doesn't make it any worse!

Here's the success message with #82 only:

Here's that same success message along with the latest patch from #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig:

Closed #2274989: Module "Install from a URL" failed as a duplicate.

Additionally, it seems like the "Archivers error" is old: #1002036: Update manager doesn't update modules: "Archivers can only operate on local files" error

We probably shouldn't reopen that issue though, better to open a fresh one I'd say since that has been closed for years.

Tested on simplytest.me with URL and upload to get some more data. It doesn't really work but the patch gets you further at least. And the Twig conversion lets you see the errors.

#83:

#83 with the latest patch from #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig:

+++ b/core/modules/system/system.module
@@ -414,34 +414,28 @@ function system_authorized_init($callback, $file, $arguments = array(), $page_ti
+ * @param bool $batch_redirect
+ *   Optional batch redirect flag, use if the page need to redirect on itself
+ *   after a batch is finished.
...
-function system_authorized_get_url(array $options = array()) {
+function system_authorized_get_url(array $options = array(), $batch_redirect = FALSE) {
+  // After a batch, we need to strip 'core/' part and set 'script' option so
+  // that we end up on the same page, as 'core/' will be our basePath.
+  $script_path = $batch_redirect ? 'authorize.php' : 'core/authorize.php';
+
...
-  $url = Url::fromUri('base:core/authorize.php');
+  $url = Url::fromUri('base:' . $script_path);

This can be replaced by using ->setOption('base_url', $GLOBALS['base_url']) but I don't know which way is the cleanest. I used that way in #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig, for the same reason i.e. because the script is in core/ directory and thus the basePath picked up by UrlGenerator is wrong.

With the changes in this patch, system_authorized_batch_process() is mis-named and mis-documented (since it doesn't do anything to process the batch).

I wonder if it's possible to keep using the batch_process() approach here (since that seems to be the only documented way to change the batch URL and redirect URL via the batch API), and leave further refactoring for a different issue (a lower priority one).

@SebCorbin the changes you made in #82 really need an interdiff, and I strongly believe those changes aren't needed. They be useful but not in this issue. If we need to change base: to $GLOBAL['base_url'] we can do that in a completely separate issue related to how urls are generated. That just makes things harder to understand what the patch is trying to fix. I'd like to keep the changes to a minimum.

Same with the Archiver issues @Cottser mentioned, they are related to the file existing in the tmp/ directory already. If you manually delete the created folders in tmp/ before uploading that issue will never happen. I'll create another issue for that.

@David_Rothstein RE #87 Yes docs needs updating.

I tried using batch_process() but the way it's documented doesn't seem to work the same, maybe you want to give it a try? I'm not a batch expert that's why I had @yched look at it in #81

Thanks @Cottser for finding that old issue, I've re-opened it for the archiver issue. #1002036: Update manager doesn't update modules: "Archivers can only operate on local files" error

I think I got this working with the batch_process() approach - see attached. Basically we just need to make sure to always use the Response object that is returned (but boy was it a pain to track down).

I tested this on a setup where the webserver owns the files and it seems to work fine. (Did not test a setup where the webserver doesn't own the files and FTP is used instead, but the changes I made for that look like a straightforward extension.)

I agree that anything remaining could be a separate issue:

1. #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig for making the results page display HTML like it's supposed to.
2. #1002036: Update manager doesn't update modules: "Archivers can only operate on local files" error for the intermittent error that prevents the form from being submitted.
3. A new issue for fixing the core/authorize.php/<none> problem (it's weird but doesn't prevent things from working, and seems like a deeper issue with how Drupal is generating these URLs).
4. Updating existing modules and themes does not work either. In this patch I have some changes related to that (since they are exactly equivalent to the module installation changes) and assumed it would work too, but actually the problem is deeper. So I filed #2426969: Dynamic redirects are no longer possible in the batch API (breaks updating existing modules or themes with the Update Manager) instead.

@David_Rothstein thanks so much! I went down that path too and even then I tried it but was fearful of the rabithole I was digging and of making those changes that you made as I thought the scope would be moving from authorize.php/Install modules to batch API fixing (which I no almost 0 about, and learned a bunch from this issue).

Re #91.3 I've posted a comment here for now, but likely will get punted to a new issue, just need the right people looking at it. #2417827-10: Evaluate and document each use of base: in core

Testing your patch out now.

+++ b/core/authorize.php
@@ -130,7 +130,13 @@ function authorize_access_allowed() {
-    $content = ['#markup' => _batch_page($request)];
+    $content = _batch_page($request);
+    // If _batch_page() returns a response object (likely a JsonResponse for
+    // JavaScript-based batch processing), send it immediately.
+    if ($content instanceof Response) {
+      $content->send();
+      exit;
+    }

I like what you changed for the JsonResponse, +1

Thanks @joelpittet. Yeah, I definitely started to think it was a rabbit hole too, but in the end it did not look like any changes to the batch API were needed so I stuck with it :)

Just attaching a new version (no actual code changes) that fixes a minor doc issue - update_authorize_run_update() uses $projects but update_authorize_run_install() uses $project.

This works great it's less code and pushes things along. The other issues are all blocked on this so I'm RTBCing this.

We need to get this change in sooner than later so we can resolve the other issues listed in #91 and as children of this issue.

+1 from a manual testing perspective, anyway :)

Manual testing confirms that the code is working and yep there are still a lot of issues to solve - nicely listed in #91.

One concern I have is that we're liable to break this again because we're obviously lacking test coverage. Is it possible to add some coverage? Maybe, at least running a batch through authorise.php?

Let me play with the patch from #93 to see if the thing is related to the patch, or a regression that got introduced in one of the recent Url changes. Based on the date that @joelpittet mentioned in #2417827-10: Evaluate and document each use of base: in core, I am not seeing any commits that could cause this, though #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route or #2229145: Register symfony session components in the DIC and inject the session service into the request object may have.

Regarding tests, it might be possible to use a mock FileTransfer method to test all the way through the batch (trying to test with a real file transfer method would require FTP access or certain file permissions on the testing system). I'm not sure if that would work or not.

But it's probably possible to just test that the user is redirected to authorize.php after submitting the install form. That would at least have caught a large part of the bug here. I'll look into that.

Test in D8 now run in sites/simplenews/ID/ multi-sites, we have full write permissions there, obviously, so if we can force the module installer to install modules there instead of the top level modules directory, we might actually be able write full test coverage here? Assuming we now support that, remember something, but don't know for sure as I never used it :)

And as a bonus, it would also be cleaned up automatically...

You're right, it looks like it does make it all the way through the batch process, due to simpletest file permissions.

It doesn't necessarily succeed in installing the module (because it tries to write to the top-level directory, which it can't necessarily do because of the file permissions there) but that's fine for testing this - we just want to make sure it makes it through the batch somehow. However, if you run this on a system where the top-level directory is writable by the web server, running this test will presumably affect your real site (will make a new test module available there). That's not ideal so maybe we should try to figure out if it's possible to make it install in the simpletest directory.

Also, bizarrely, it is trying to install it in the top-level /themes directory (not the top-level /modules directory), even though it's a module...

Besides the issues above, this test seems to be working. To avoid notices in the tests I needed to include some changes to the theme function also (which are being made in a better way in #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig) - see the interdiff.

Sorry that I couldn't be quicker on this issue :-/

What made me hesitate a bit was that I don't think we really need to (nor should ?) run the batch in authorize.php in the case that doesn't need to go through FileTransferAuthorizeForm for authentication (i.e the case where batch runs directly at the submit of UpdateManagerInstall form because the target directory is writable directly).
In this case, the user never sees any form in authorize.php, why would it see authorize.php just for the batch progress, in a page that looks visually different than what comes before and after ?

But well, we can sort this out in a separate issue, if at all. The new approach works for me too. I wasn't aware of $form_state->setResponse() :-)

@David_Rothstein see #2413695: Modules getting installed in the theme directory if they don't have a *.module file for the themes directory installation of a module issue. Seems to happen when the info file doesn't have a type. page_manager does this now.

In this case, the user never sees any form in authorize.php, why would it see authorize.php just for the batch progress, in a page that looks visually different than what comes before and after ?

It doesn't look visually different from what comes after, since what comes after always uses authorize.php to display the results (it's basically what's shown in the screenshot in #83).

To be honest, I'm not sure that having authorize.php as a standalone script actually serves any real purpose, but as long as it does exist it makes sense to use it consistently. See also #609728: Skip authorize.php step if webroot files are owned by the httpd user (where the original goal in the issue summary - despite what the issue title says :) - was to make this use authorize.php to make it as consistent as possible with the case where FTP or similar credentials need to be entered).

@David_Rothstein see #2413695: Modules getting installed in the theme directory if they don't have a *.module file for the themes directory installation of a module issue.

Good call - yes, that's almost certainly it. The tarball I included in that patch contains an .info.yml file which identifies it as type "module", but no .module file. Sounds like we don't need to worry about it here then.

I wish we'd just killed this screen. It barely worked in D7, and in D8 will be completely incompatible with the composer workflow, which means that it won't be able to install any module that has an external library dependency.

@bojanz not sure what you mean or how that's related. If this is "incompatible with the composer workflow" then so is going to a drupal.org project page, downloading a tarball, and manually putting it on your site.... the two are technically equivalent.

OK, I wanted to see if it was possible to deal with the issue mentioned in #100 (module not guaranteed to actually install during the tests, and when it is it leaks into the parent site).

Here's what I came up with, and it seems to work. The tests are now able to go through and completely install a new module with the Update Manager in sites/simpletest, which is invisible to the parent site and cleaned up afterwards.

This was pretty complicated and involved changes beyond just the tests, so if it's going to slow the patch getting in significantly I think we could go back to one of the earlier patches here (probably #93 which has no tests at all) and then move this code to a followup issue. But it will be great to have the Update Manager actually testable and I think the code I have here does that.

The attached interdiff is for everything from #100 except the .tar.gz files (I created a new .tar.gz file, with a .module file inside it to deal with #2413695: Modules getting installed in the theme directory if they don't have a *.module file, and also to work around a problem that occurred because the previous module name, "bbb_update_test", was already used elsewhere in the codebase).

I'm fine with the other changes but most of them don't affect me (yet:P)

Although regarding the theme function changes:

+++ b/core/includes/theme.maintenance.inc
@@ -129,7 +129,10 @@ function theme_authorize_report($variables) {
-        $items[] = drupal_render($authorize_message);
+        $items[] = array(
+          '#markup' => drupal_render($authorize_message),

@@ -157,12 +160,6 @@ function theme_authorize_report($variables) {
-  $success = $variables['success'];
-  if ($success) {
-    $item = array('data' => array('#markup' => $message), 'class' => array('authorize-results__success'));
...
-  return $item;
+  $item = array('#markup' => $message);
+  return drupal_render($item);

Are the changes in the theme function necessary? If so I'll clean them up some because we are attempting to avoid calling drupal_render, because it's deprecated and we are attempting to avoid early rendering.

I believe they were necessary to avoid PHP notices during the tests (when the tests hit the last step of the authorize.php batch process).

The main change that was needed was to force the theme function to return an HTML string (not really clear how it ever worked returning an array). Please do feel free to clean them up if you want to. Although, I kind of see this as a stopgap fix anyway (with the real fix coming in #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig which removes all this code)... is introducing a drupal_render() call in a theme function so bad when theme functions themselves are deprecated? :)

Manually tested the patch (under mentoring!!) - install worked, no links to refer back to site (as per image attached), module subsequently shown in list of modules.

Thank you for manually testing @miniwebs2!

No problems - only hope the issue gets resolved eventually, am sure it will but is a must-have for Drupal 8 and those who work via front end UI. I'm not a coder, developer or php person and although I can install modules and themes via the files backend, the UI is there for a reason, to work! Will keep an eye out for updates.

@David_Rothstein#106
Downloading a tarball from drupal.org still allows you to fetch additional PHP dependencies manually.
This screen will try to run the install right away, not leaving space for preparation steps, no? (Disclaimer: haven't used it in years)
Hence my "it needs to die" comment, there's simply no way to make it compatible with the part of D8 contrib that decides to use libraries (and I'm guessing it will be a significant percentage).

Our work to make Drupal "Composer ready" goes directly against the goal of allowing UI-based module install. But anyway, I'm bikeshedding, just thought it was worth mentioning.

David I have no idea what's behind the 'Composer ready' setup (as mentioned I'm not a backend person), but if the goal within that is to disallow UI module install, how can a front end user then install a module if there is no UI for it. Will there be another way to install modules from the front end?
The screen does not even try to run an install as it is at present, either via a URL or uploading a module - it just resets the UI back to the same 'Install a Module' page. Nothing changes.

Thanks for reviving this.

@joelpittet, do you think this might be good to go, given my comment in #110 that the only deprecated function call is being introduced inside another, already-deprecated function anyway?

Would be great to get this in and then move on to #2426969: Dynamic redirects are no longer possible in the batch API (breaks updating existing modules or themes with the Update Manager)... (I already have a patch there, but it's waiting on this one before it's possible to fully work and write tests for it.)

This screen will try to run the install right away, not leaving space for preparation steps, no? (Disclaimer: haven't used it in years)

It won't, actually. It just downloads and extracts the package and then links you to the main modules page to decide which module(s) you might want to enable.

But even if it did, it wouldn't matter for your use case. If a module has additional dependencies (outside libraries that need to be downloaded or anything else) it should implement hook_requirements() to prevent the module from being installed before those dependencies are in place. This is no different in Drupal 7 vs. Drupal 8.

Of course it would be a really neat feature if the UI-based module installer could figure out those dependencies and download them automatically for you, but even without that I think the feature is very useful (and works perfectly fine for the majority of Drupal contrib projects).

when I applied the patch 2042447-107-install-module.patch and typed the url in the "install from a url" field it then brought this error message

In general this patch looks pretty great!

1. +++ b/core/authorize.php
   @@ -130,7 +130,13 @@ function authorize_access_allowed() {
   +    if ($content instanceof Response) {
   +      $content->send();
   +      exit;
   +    }
   

   it would be nice to not introduce a new exit; call, at least a follow up here would be great.

2. +++ b/core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
   @@ -333,13 +337,18 @@ protected function setConnectionSettingsDefaults(&$element, $key, array $default
   +   * @return mixed
   +   *   The result of running the operation. If this is an instance of
   +   *   \Symfony\Component\HttpFoundation\Response the calling code should use
   +   *   that response for the current page request.
   

   So is this a Response or a render array?

I am still having this problem in my new installation of Drupal 8 Beta 10 (at Drupalcon 2015 L.A.). I cannot install contrib modules by file load or URL reference. Nothing happens. And I cannot see that there is anything wrong with configuration. I have to unzip the module file and place it in the "modules" folder. At first I was wondering whether this was by design (disable things that are in flux), but it looks from this page like it is a recurring bug. (I am just getting around to updating bleeding edge Drush, or I might have tried using that.)

@pkosenko you've come to the right place, we are trying to fix it here. Did you apply this patch from @David_Rothstein in comment #107?

That is our best solution as of yet to resolve this issue. I was planning to review this during the con but got distracted by a whole lot of other shiny issues.

@joelpittet tried the patch by @David_Rothstein on Simplytest.me works fine from what I can see only error is the mkdir but that will be permissions as its on a public setup.

Likely simplytest.me isn't going to let you install modules willy nilly;)

@joelpittet yeah thats what I thought, going to run it on my vagrant setup to see if it works

Reroll. Checked out b31bbb084ab0d5e5a9ce0985db1dccaf39242bf9, applied patch, and then rebased against 8.0.x:

First, rewinding head to replay your work on top of it...
Applying: 2042447-107-install-module.patch
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
M	core/lib/Drupal/Core/Updater/Module.php
M	core/lib/Drupal/Core/Updater/Theme.php
M	core/lib/Drupal/Core/Updater/Updater.php
M	core/lib/Drupal/Core/Updater/UpdaterInterface.php
M	core/modules/update/src/Form/UpdateManagerInstall.php
M	core/modules/update/src/Form/UpdateReady.php
M	core/modules/update/src/Tests/UpdateUploadTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/update/src/Tests/UpdateUploadTest.php
Auto-merging core/modules/update/src/Form/UpdateReady.php
Auto-merging core/modules/update/src/Form/UpdateManagerInstall.php
Auto-merging core/lib/Drupal/Core/Updater/UpdaterInterface.php
Auto-merging core/lib/Drupal/Core/Updater/Updater.php
Auto-merging core/lib/Drupal/Core/Updater/Theme.php
Auto-merging core/lib/Drupal/Core/Updater/Module.php
Auto-merging core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php

No manual merges needed.

I can reproduce these locally. Can't track down why the exceptions are occurring, but the first is coming from the call to Drupal\Core\Updater\Module::isInstalled(), from \Drupal\update\Form\UpdateManagerInstall::submitForm(). A quick glance of two months of commits didn't turn up anything...

Hm, those are due to #1081266: Avoid re-scanning module directory when a filename or a module is missing which was committed a few weeks ago. I'm taking a look now to see if I can figure it out.

I will also address the review in #120.

Hm, that is messy - drupal_get_path() does not work anymore to check if a module exists in the filesystem, and there isn't an obvious replacement. The attached seems to work though.

Responding to #120:

it would be nice to not introduce a new exit; call, at least a follow up here would be great.

Since this occurs in authorize.php (which is a standalone script) it is not really any different than the implicit "exit" which happens at the bottom of the script. The new exit call being added here is basically the same as used elsewhere in that file (once it knows what it wants to send, it sends it and exits - or it reaches the end of the script and sends there).

So is this a Response or a render array?

I think it's actually a Response or null - it should basically be the same as what batch_process() returns since ultimately it is processing a batch. In the attached, I've updated the @return type hint throughout the patch to match the one in batch_process().

I also made one other change: I added some documentation to UpdateRootFactory::get() to explain why we have test-specific code in there (rather than relying on tests to override it). Someone will probably be curious about that at some point.

+ * @return \Symfony\Component\HttpFoundation\RedirectResponse|null
+ *   The result of processing the batch that updates the projects. If this is
+ *   an instance of \Symfony\Component\HttpFoundation\Response the calling code
+ *   should use that response for the current page request.

Oops, mismatch there (RedirectResponse vs Response).

In reality I think it's always RedirectResponse if it comes from batch_process() but we can't 100% guarantee that anyway, and Response is more generic and makes more sense here. The attached just updates the documentation to use that.

Regarding the interdiff in #133, yes checking the state data instead of drupal_get_path() should be fine

#134 allowed me to install and uninstall Testing module.

Thanks, David_Rothstein!

Hi, apologies if I have circumvented standard protocol here.
I have just installed D8 Beta 11 in a live site for testing a D7 module upgrade and have encountered the dreaded 'module not found' error after attempting an install via the upload mechanism - no files were copied into place.
I manually applied all the coding changes mentioned in the #134 patch and now i get an error which seems to stem from $container->get('update.root').
Is it possible I missed another requirement from an earlier patch to get this working? I am very new to D8 code..
here is the complete error:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "update.root". in Symfony\Component\DependencyInjection\Container->get() (line 317 of core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).

Drupal\update\Form\UpdateManagerInstall::create(Object)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('\Drupal\update\Form\UpdateManagerInstall')
Drupal\Core\Controller\HtmlFormController->getFormObject(Object, '\Drupal\update\Form\UpdateManagerInstall')
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Symfony_Component_HttpKernel_HttpKernel_Proxy->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_StackMiddleware_Session_Proxy->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\DependencyInjection\Container\prod\Drupal_Core_StackMiddleware_KernelPreHandle_Proxy->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

I also wasnt sure if this should be a comment or a review. Its my first post here.

@Gilbert Rehling You likely need to rebuild the container. drush cr does that or just delete it from /sites/default/files/php/service_container folder. It will rebuild automatically on the next page load.

@joelpittet Thanks Joel - you were dead right. That's my first lesson in Drupal Core requirements. But, it completed successfully with an error.
URL where process finished: /core/authorize.php/
I can also confirm that the module appears to have installed correctly into the /modules/ folder.
Screenshot included:

The previous patch no longer fully applied - maybe that was part of the issue in #139? This one works with no errors for me.

Or possibly this was the problem in #139:

I manually applied all the coding changes mentioned in the #134 patch and now i get an error

I would recommend using https://www.drupal.org/patch/apply to apply patches directly. Trying to make the coding changes manually is really likely to result in mistakes - and also really time-consuming :)

The above is just a straightforward reroll of #134 by the way. The only reason the patch didn't apply anymore was that some of the surrounding context changed.

Updated the update.root definition to match the way core now defines app.root.

RTBC, Looks great!

Do we have an existing issue for the authorize.php/<none> bug?

What about the bullet points that show plain text in "Next steps", is that an issue isolated to that page?

That is double-escaping and likely can be dealt with as the rest of double escaping issues, so should be follow-up.

1. +++ b/.htaccess
   @@ -138,6 +138,8 @@ AddEncoding gzip svgz
   +  # @todo Remove this once the <none> bug is fixed.
   +  RewriteCond %{REQUEST_URI} !/core/authorize.php/<none>$
   

   needs issue URL

2. +++ b/core/includes/theme.maintenance.inc
   @@ -124,7 +124,10 @@ function theme_authorize_report($variables) {
   -        $items[] = drupal_render($authorize_message);
   +        $items[] = array(
   +          '#markup' => drupal_render($authorize_message),
   +          '#wrapper_attributes' => array('class' => $log_message['success'] ? 'authorize-results__success' : 'authorize-results__failure'),
   +        );
   
   @@ -152,12 +155,6 @@ function theme_authorize_report($variables) {
    function theme_authorize_message($variables) {
      $message = $variables['message'];
   -  $success = $variables['success'];
   -  if ($success) {
   -    $item = array('data' => array('#markup' => $message), 'class' => array('authorize-results__success'));
   -  }
   -  else {
   -    $item = array('data' => array('#markup' => $message), 'class' => array('authorize-results__failure'));
   -  }
   -  return $item;
   +  $item = array('#markup' => $message);
   +  return drupal_render($item);
   

   nit, no reason in $message variable
   also that's change makes this function unneeded

3. +++ b/core/lib/Drupal/Core/Updater/Module.php
   @@ -31,20 +31,30 @@ class Module extends Updater implements UpdaterInterface {
   -    if ($relative_path = drupal_get_path('module', $this->name)) {
   +    if ($this->isInstalled() && ($relative_path = drupal_get_path('module', $this->name))) {
   ...
   -      $relative_path = 'modules';
   +      $relative_path = $this->getRootDirectoryRelativePath();
   ...
   +  public static function getRootDirectoryRelativePath() {
   +    return 'modules';
   
   +++ b/core/lib/Drupal/Core/Updater/Theme.php
   @@ -31,20 +31,30 @@ class Theme extends Updater implements UpdaterInterface {
   -    if ($relative_path = drupal_get_path('theme', $this->name)) {
   +    if ($this->isInstalled() && ($relative_path = drupal_get_path('theme', $this->name))) {
   ...
   -      $relative_path = 'themes';
   +      $relative_path = $this->getRootDirectoryRelativePath();
   ...
   +  public static function getRootDirectoryRelativePath() {
   +    return 'themes';
   
   +++ b/core/lib/Drupal/Core/Updater/UpdaterInterface.php
   @@ -43,6 +43,14 @@ public static function getProjectName($directory);
   +   * Returns the name of the root directory under which projects will be copied.
   ...
   +  public static function getRootDirectoryRelativePath();
   

   this is exactly class constant - I see no reason to implement static method for it

per #150

1. Regarding the @todo, see #92 above and #2417827-10: Evaluate and document each use of base: in core, but yeah, now that we have two of them for the same issue (and the new one, unlike the first, isn't something anyone will automatically know to remove once this is fixed) we probably need to follow up on that.

   One thing I also thought I saw earlier, some of the URLs generated at the end of the module install process were pointing to places like /authorize.php/admin/modules so the URL generation from this script has some broader problems... Could be related to the above @todo, but either way, it's a separate issue.

2. How about we remove the unneeded $message variable, but leave the rest alone. As mentioned before, this is all getting removed in #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig (the issue which will fix the double-escaping also) so we don't need to worry much about it here.
3. If we made it a class constant, wouldn't that mean it couldn't be documented properly in the UpdaterInterface interface? We are requiring that all classes which implement the interface implement this method, and I don't think PHP lets you enforce that with class constants.

   Also I think it would be nice to keep it a method just in case anyone ever wants to make it more dynamic someday (e.g. make the Update Manager work with multisite).

#153: Should we not open a dedicated issue for that instead and point to that, pointing to comments is usually not done.

created #2526392: authorize.php redirects to authorize.php/<none> at the end of the batch run

Back to RTBC then as all feedback has been addressed.

Added a reference to that issue in the previous @todo also, and changed the reference to a URL. Also uploading a final version that the testbot will actually do something with, so there's no confusion :)

I only made very minor changes, so leaving this at RTBC.

+++ b/core/includes/theme.maintenance.inc
@@ -151,13 +154,6 @@ function theme_authorize_report($variables) {
+  return drupal_render($item);

Can we update this?

#158, there is a separate issue for that: #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig.

Tests has been addressed on https://www.drupal.org/node/2042447#comment-9629107

Is this bug still not patched in beta 12? I got a very similar problem (page refresh, nothing else on both uploading a module file (admin_menu module) and entering a module file URL).

However, the following information was logged about it:
<em class="placeholder">User warning</em>: The following module is missing from the file system: admin_menu in <em class="placeholder">drupal_get_filename()</em> (regel <em class="placeholder">255</em> van <em class="placeholder">/home/sjxofphr/domains/domainobfuscated.com/public_html/core/includes/bootstrap.inc</em>).

#162, yes this is still an outstanding bug in all versions of Drupal 8. Until this get committed (status will change to closed), you may see this problem. After it gets committed, we will start looking into followup issues.

Adding an issue that is postponed on this one. (There are actually a pretty large number of issues that are either directly or semi-directly postponed on this.)

Um it shows up in the related issues, but not in the above comment for some reason. It was #842620: Update manager can't install modules using FTP due broken FileTransferAuthorizeForm

Edit: That's because it was already added as a related issue... over a year ago.

Okay!

My go-to testing platform for Update Manager in the past has been DreamHost, but unfortunately they can no longer install D8 thanks to the MySQL 5.5 requirement. :\ I also tried simplytest.me (tested against D7) but it also failed:

Installation failed! See the log below for more information.
token

Error installing / updating
File Transfer failed, reason: Cannot create directory /home/ddhk/www/sites/all/modules/token.

Ok, let's go for the nuclear approach:

----

First, let's do a review of what D7's update manager did.

1. Go to admin/modules, click "Install new module."

2. Next, either enter a URL to a project's tarball, or upload one. Let's keep it simple and add a module that has no dependencies, and has both a D7 and D8 version: Token.

3. After a brief tarry in Batch API, you end up at a success message (or a failure message as indicated above). You're given the option to install another module, enable newly added modules, or visit the administration page.

4. If you click on "Enable newly added modules," you're taken to the admin/modules page. (Note: it would be nice here instead if it used an #ID to jump you down to the right spot automatically; I'm sure we have an issue for that somewhere.) There you'll see Token:

5. Check it, save configuration, and you're on your way. (Verify at admin/help/token.)

Next up... what happens in 8.x HEAD.

Unpatched, here's what happens in 8.0.x right now:

1. First, hit the Extend page and the Install new module button. So far, so good.

2. Get the same screen as above with either the URL or browse file button. Install token D8 dev release.

3. Click install, end up back at the same page with no error message. Womp-womp. (Annoying empty box caused by #2547127: [regression] Empty messages container appearing when a Notice occurs, not part of this issue.)

So that's obviously not good. Now let's try the patch...

With patch, first two screenshots are the same. Hit "Install" and....

Quick batch API and...!

Forbidden error. :(

Forbidden

You don't have permission to access /core/authorize.php/<none> on this server.

So something is goofed up still. I'm guessing this was covered by recent comments. I'll take a look in a sec.

Ok, looks like that fix was split off into #2526392: authorize.php redirects to authorize.php/<none> at the end of the batch run for some reason, stay tuned...

@webchick that error should have been covered by the patch as it adds this line to .htacccess:

RewriteCond %{REQUEST_URI} !/core/authorize.php/<none>$

Can we figure out why that's not getting picked up by your webserver somehow?

If there was an issue with the patch, automated tests should also have caught that:

+++ b/core/modules/update/src/Tests/UpdateUploadTest.php
@@ -56,6 +57,25 @@ public function testUploadModule() {
+    // Check that submitting the form takes the user to authorize.php.
+    // @todo Remove <none> once https://www.drupal.org/node/2526392 is fixed.
+    $this->assertUrl('core/authorize.php/<none>');
+    // Check for a success message on the page, and check that the installed
+    // module now exists in the expected place in the filesystem.
+    $this->assertRaw(t('Installed %project_name successfully', array('%project_name' => 'update_test_new_module')));

Take 30 or whatever.

Same screenshot 1 and 2 from #167.

Token is already installed.

LOL :)

rm -rf modules/token

ONE MORE TIME.

Token is already installed.

...? :(

Ok, going to blow EVERYTHING away and start all over again....

Oops, cross-post with #170. Sure, I would love to figure that out. Can you hop into #drupal-contribute on freenode IRC?

Just did a manual test to confirm. Installing the token module via the drupal.org URL using Update Manager doesn't give me any Forbidden error on HEAD+patch:

So while I wait for git clone to finish, I will just say that between:

diff --git a/.htaccess b/.htaccess
index b26c63b..c528352 100644
--- a/.htaccess
+++ b/.htaccess
@@ -147,6 +147,8 @@ AddEncoding gzip svgz
   # security, you can replace '!/' with '!^/'.
   # Allow access to PHP files in /core (like authorize.php or install.php):
   # Allow access to PHP files in /core (like authorize.php or install.php):
   RewriteCond %{REQUEST_URI} !/core/[^/]*\.php$
+  # @todo Remove this once https://www.drupal.org/node/2526392 is fixed.
+  RewriteCond %{REQUEST_URI} !/core/authorize.php/<none>$
   # Allow access to test-specific PHP files:
   RewriteCond %{REQUEST_URI} !/core/modules/system/tests/https?.php
   # Allow access to Statistics module's custom front controller.

...and:

diff --git a/core/modules/system/system.module b/core/modules/system/system.module
index 732e27b..332c6c1 100644
--- a/core/modules/system/system.module
+++ b/core/modules/system/system.module
@@ -460,7 +460,7 @@ function system_authorized_run($callback, $file, $arguments = array(), $page_tit
 function system_authorized_batch_process() {
   $finish_url = system_authorized_get_url();
   $process_url = system_authorized_batch_processing_url();
-  return batch_process($finish_url->toString(), $process_url);
+  return batch_process($finish_url->setAbsolute()->toString(), $process_url);
 }
 
 /**

...I would far, far prefer the latter fix. I don't understand putting in a one-line workaround for a one-line fix?

Ok, this time... with 100% fresh git cloned goodness...

IT WORKED!!

Er, well. ;)

- "Authorize file system changes" as a title?
- Escaped HTML for links?
- Links going to /core/authorize.php/admin/modules and the like, which is a 403? (Apache error; same as #168).

There's... definitely still some stuff to clean up here before we can call this feature shippable. :\ But not really related to this patch.

The bottom line is: THIS FIXES THINGS SO IT WORKS! YAY! :D

Next, onto the code review...

Btw, according to @sebcorbin, the auto-escaping issues are fixed by #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig.

YAY!

That most recent patch incorporates the one-line fix from #2526392: authorize.php redirects to authorize.php/<none> at the end of the batch run mentioned in #174. With this change, all other adjustments (I think. It's 2am...) are isolated to authorize.php / Update Manager, rather than leaking into "core core" files. While I conceptually agree with David_Rothstein in #91 that it's a separate issue, nevertheless it's required in order for this feature to function for end users.

Otherwise I saw nothing to complain about. Added tests FTW!!

Committed and pushed to 8.0.x. YEAHHHHH!! Thanks so much for all of your hard work on this one!! :D

Thanks! Created a follow-up issue at #2547667: Fix broken page title Update Manager (authorize.php)