[10.x] Update fixture hash to match testing cost

[timacdonald]

Matches the lower cost for unit testing.

[bastien-phi]

I think this should be updated with

'password' => Hash::make('password'),

Because changes from laravel/framework#48791 causes

   INFO  Seeding database.  


   RuntimeException 

  The hash does not match the configured hashing options.

  at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php:1328
    1324▕             return Hash::make($value);
    1325▕         }
    1326▕ 
    1327▕         if (Hash::needsRehash($value)) {
  ➜ 1328▕             throw new \RuntimeException('The hash does not match the configured hashing options.');
    1329▕         }
    1330▕ 
    1331▕         return $value;
    1332▕     }

during database seeding

[driesvints]

No, the entire idea is that we don't need to run the hasher for each seed operation which would slow down tests, etc. If this PR fails seeding because of laravel/framework#48791 then we might need to re-consider that @timacdonald?

[Jubeki]

What about the database seeding, where the cost should be 12 instead of 4?

[timacdonald]

I've just pushed a change so that the lower cost is only used while running unit tests.

[mfn]

May I suggest if this gets accepted there should be comment as to why this; I bet ppl will be confused by this. I know I am when I look just the code, even knowing the intention of the PR :-}

[driesvints]

Wait, now I'm a bit confused why this was a problem @bastien-phi? In what situation would the lower cost be an issue for you?

[bastien-phi]

The problem is not about a lower or higher cost, the problem comes from a de-synchronization between the provided hash and the configured bcrypt rounds parameters.

If the hash and the configuration do not match, the hashed cast will throw a RuntimeException.

Current implementation is correct but I agree with @mfn, it could be tricky to understand why the implementation is like that (especially in user-land code).

I stand by it could be cleaner to use Hash::make('password') but it comes with a cost :

• ~ 0.8ms with round = 4 (during tests)
• ~ 170ms with round = 12 (during seeds)

[driesvints]

@bastien-phi no I mean why can't we just switch to $2y$04$Pnnh9Ay6phg2joDy5EDzE.icSkpTHvQFew2kSGPK06PyUDwpBuORG? What would break?

[bastien-phi]

Oh sorry I wasn't clear in my first message. If we define the hash directly with '$2y$04$Pnnh9Ay6phg2joDy5EDzE.icSkpTHvQFew2kSGPK06PyUDwpBuORG', it would cause a RuntimeException during database seeding because of the changes in laravel/framework#48791 as the rounds in the hash (4) will mismatch the rounds define in the application (12 by default).

[driesvints]

@bastien-phi right, thanks.

@timacdonald would it be an idea to hardcode the rounds to 4 when running tests here:

https://github.com/laravel/laravel/blob/10.x/tests/CreatesApplication.php#L18

We used to do that in the past iirc. Then we can just define $2y$04$Pnnh9Ay6phg2joDy5EDzE.icSkpTHvQFew2kSGPK06PyUDwpBuORG in the factory and @mfn's correct remark of things being confusing would be resolved.

[bastien-phi]

@driesvints don't forget that the developers can change hash algorithm and use argon instead of bcrypt...

[driesvints]

@bastien-phi in that case it’s best that they update the hash itself.

[valorin]

@timacdonald would it be an idea to hardcode the rounds to 4 when running tests here:
https://github.com/laravel/laravel/blob/10.x/tests/CreatesApplication.php#L18
We used to do that in the past iirc. Then we can just define $2y$04$Pnnh9Ay6phg2joDy5EDzE.icSkpTHvQFew2kSGPK06PyUDwpBuORG in the factory and @mfn's correct remark of things being confusing would be resolved.

It's defined in the phpunit.xml as an override to the env var:

<env name="BCRYPT_ROUNDS" value="4"/>

(https://github.com/laravel/laravel/blob/10.x/phpunit.xml#L22)

If the developer changes this value and things break, it should be obvious enough that what broke it. Same as with switching to Argon.

[driesvints]

@valorin ah good call. In that case there shouldn’t be anything wrong with just using the rounds 4 version.

[bastien-phi]

What about

class UserFactory extends Factory
{
    public static string $password;

    /**
     * Define the model's default state.
     *
     * @return array<string, mixed>
     */
    public function definition(): array
    {
        return [
            'name' => fake()->name(),
            'email' => fake()->unique()->safeEmail(),
            'email_verified_at' => now(),
            'password' => static::$password ??= Hash::make('password'),

?

It brings simplicity and also makes sure that generated hash is consistent with current configuration (algorithm and parameters)

[taylorotwell]

@bastien-phi updated to your suggestion

[Softwaerewolf]

Even though this has already been merged, I wanted to leave a note that using Hash::make('password') in UserFactory was necessary for tests to pass when using pgsql.

It would treat the hard coded hash as plain text input and get rehash, then compare the rehashed value to the hardcoded hash and fail. Only using Hash::make or providing an unhashed value allowed the test to pass.

Not sure if this points at some underlying inconsistency between mysql and pgsql drivers?

[valorin]

Sounds like you've got hashed casting enabled on the model - the database driver shouldn't be doing any hashing.

[Softwaerewolf]

Sounds like you've got hashed casting enabled on the model - the database driver shouldn't be doing any hashing.

Only if it's enabled in the default Laravel project.

I only did the bare minimum to set up pgsql instead of mysql and everything else is standard.

[Jubeki]

The hashed cast is enabled by default:

laravel/app/Models/User.php

Line 43 in 2254e8e

'password' => 'hashed',

[Softwaerewolf]

Shouldn't this also affect mysql driver then? The problem is that the behavior is different.