DevTools Require - Validate credentials

[promatik]

This is the result;

This was a lot of work, because DevTools was always installing, even after removing the credencials and my private key (to avoid installing from source), composer has some kind of cache of my key (?🤷‍♂️).
I had to test this on Windows linux subsystem.

Sorry for the huge amount of changes, not great for code review, but I had to separate everything in functions to avoid code repetition.

[pxpm]

@promatik please take a look at this reply

I've added the code here that I used to log the error into a custom log file, not sure if it's 100% worth or we simply log in the default user channel, but I think it's safer and 100% guaranteed that the error would be in that file and the logger is built on run-time so no need to be correctly configured in the app confg().

I think this is the only place where we need to write to the file? Since we catch the credentials error. a possible network error, and this "composer error" is the last we need to catch right ?

About the display, I am just saving $buffer directly to log file, maybe we prettify it ?

Let me know,
Pedro

[promatik]

To sum up;
This PR is now not only validating the credentials, but all the other errors we found.
The ones we don't know about and we're not expecting, are being logged on the error log, nice idea @pxpm ✌

[scrutinizer-notifier]

The inspection completed: 506 Issues, 81 Patches

[tabacitu]

I'll merge this as soon as it has @pxpm 's green light.

[tabacitu]

@pxpm heads-up - this is waiting for your green light.

[pxpm]

Sorry for taking a bit of time to come back here.

After review I think we still have something to address here.

Look at this screenshot:

• new laravel instalation
• require backpack/crud --prefer-source
• change to this branch and run the command

My guess is that composer is not really using the values we provide but a cached version otherwise the password worng would have not worked.

Next all of the steps above but I clear composer cache before:

Fine .. I guess .. until I try again: php artisan backpack:install

No asking about devtools instalation ? Hum .. Somehow vendor/backpack/devtools exists as an empty folder. I manually delete it and run the command. Gave the correct credentials and everything worked.

Went back to the project where I am running old dbal version, run the command and as expected it failed! (very nicely @promatik 👍 ), just a minor issue I found is that the command would retry again when failing, and I don't think that should happen because it's not something that is going to be "fixable" on a second try.

So I think there are two things that we need to address here, the composer cache and the check if package exists in the Install command of Backpack Crud.

if (! file_exists('vendor/backpack/devtools')) { //we need a different way to check here, probably check if the service provider exists or something ? 
            $this->box('Did you know about Backpack DevTools?');
            ....
            }
        }

Let me know @promatik

Pedro

[tabacitu]

Good catch @pxpm ! My thoughts on this:

• I don't we can check that the service provider is loaded, in the same runtime. It may be possible to check that directory is there and is not empty? But the question is - why did you have an empty devtools directory there - did you generate it or did Backpack in a failed installation attempt? If it's the second, yeah, we need to account for that.
• I agree that it probably shouldn't try twice if there's a Composer error. But it's not that big of a deal to me.

[promatik]

Hi @pxpm! Great review 🙌

Ok, the first issue, about credentials, I think we can't do much about it, here is the process flow;

1. Installation process checks if there exists credentials with the default command composer config http-basic.backpackforlaravel.com – it checks locally and composer cache.

• If it fails to run the command (to really execute the command on the system), it will read the local auth.json file.
• If the command is executed, we read data from composer variables, if any, either local or global cache.
  Note: this just checks if variables exists, not if are correct.

2. If there are details, all good, we try to install. If not, we ask the details and write to the auth.json (not the global cache).

I think there are 2 possible scenarios for your case;

1. You are having issues running the composer config command, so it doesn't have access to the the details, and asks you for new, but ends up, internally, using the right global credentials.

2. Or maybe, first it reads the details, and found none, so it asked you for it and store it in auth.json.
   It then tries the download, composer failed internally with your auth.json details, but succeeded somehow with your github access (I don't know how it does, but it works, @tabacitu knows better 😅)

If case 2 is happening, DevTools will never know which credentials were used, and in case the local auth.json has pxpm - wrongpass it will keep the wrong credentials there...

---

Regarding problem 2, yes, that is an "unfixable" error, and that's why I added this code;

if (strpos($buffer, 'Your requirements could not be resolved') !== false) {
    $this->progressBar->advance();
    $this->error($buffer);
    $process->stop(0);
}

Note the $process->stop(0);, since you hit the 'Your requirements could not be resolved' it should have stopped the installation. I'm sure I stumbled in this situation too, and I think it worked for me 🤦‍♂️ I'm not sure, so I'll try once again.

• Avoid retry installation in case of "fatal" errors.

---

Regarding problem 3, it doesn't happen to me! what a crappy situation 😅
I'll figure out a way, maybe we can check of a file inside DevTools folder, it's odd, but it would solve the problem since we can't use class_exists yet.

• Improve DevTools installation verification.

---

I'll update you as soon as possible ✌

[tabacitu]

Guys... could you please remind me... why did we decide to do this? Why do we need to move the user&pass prompt from Composer's screen to our own screen, and validate it? The first post doesn't really say that, nor do the replies. And stupid old me doesn't remember.

Thanks!

[pxpm]

@tabacitu because some errors if not handled would produce "silent" instalation fails, like, for example a conflicting version in your composer.json when trying to install devtools.

👍

[tabacitu]

Closing in favor of #4559 which will get merged soon

[pxpm]

I don't think this tackles this issue? Does it? Is by sending users to check logs?
Haven't tested, just looked at code..
Cheers

[promatik]

@pxpm this code was mainly moved from this PR to to #4559.
So, yes, missing stuff here must be there too.
(the part on retrying the installation was removed)

Although this (now #4559) doesn't cover every possible error, it is better to have an installer than don't have it 🤷‍♂️

We may fix errors we (and devs) find from now on, but I think it may be a work in progress ...

[pxpm]

Got it 👍