Impossible Migration to v2?

[kohlerdominik]

Hi

from what I can see, it seems impossible to migrate from v1 to v2, as v2 dropped the functionality to make reproducable keys? But maybe i just don't see how to do it...

So, in version one, we had the functionality to have a user-defined salt-generator by using makeHashedIdSalt() on each model. With this is was possible to replace the generic, laravel-key-based salt and use reproducable salts. But now, this method was removed and it was hardcoded in Repository::get() without any way of custimization. Is that correct?

Why this is an issue:
Assume you have an URL /device/MyH4sh1D stored to your bookmarks, as this is your personal device and you check it occasionally. Now a dev upgrades the package, which will change the "salt-generator", and ultimately, every link to your application is broken. This also applies for example if mails with a link were sent out or if the user somewhat stores the HashId somewhere.

[kohlerdominik]

Hi @veelasky

Can we get some feedback on this?

[veelasky]

@kohlerdominik sorry for the late response.

yes it was intended.

in v2, you can override the getHashKey() method to have the same effect for custom-generated key (salt).

[veelasky]

sorry for the inconvenience,,, I was not expecting anyone would override makeHashedIdSalt() on version 1, now it simply removed, and provide developer to add the key to the model itself in version 2 using property like so:

class User extends Model 
{
    use HashableId;
    
    // additional property to look for custom salt.
     protected $hashKey = 'custom-salt-for-model';
}

but as I said, you can always override the underlying getHashKey() method to have the same effect:

class User extends Model
{
    use HashableId;
    
    public function getHashKey(): string
    {
        return method_exists($this, 'makeHashedIdSalt') ? $this->makeHashedIdSalt() : parent::getHashKey();
    }
}

[kohlerdominik]

Hi @veelasky

I tried that when i wrote the issue. For some reason i do not exactly remember, also by overwriting getHashKey() I was not able to reproduce the keys i made in Version1.

There are a couple of Bookmarks and Emails out there, referencing to the V1-IDs, so for us it's important that we can use the exact same logic to generate keys as in V1.

I think it was this line, which is not overwritable, so it's not possible to add custom key generation logic. The particular issue might be, that the AppKey no longer can be replaced by a custom string (like we did it to make reproducable keys in production, stage and local environments).

[veelasky]

ah yes.. you're right.

I'll try to find a way to accommodate your issue without regressing back for current release.

[kohlerdominik]

Cool, thanks. We're looking forward to it.

As input: one of the patterns often seen in laravel is the possibility to inject a closure like:

$generator = function(string $key) {
    substr($key, -10) . 'addSalt';
}

Veelasky\LaravelHashId\Repository::generateKeyUsing($generator);

As I think the Repository is registered as singleton, I think this should be fairly easy to do. But you might find a better approach.

[veelasky]

@kohlerdominik

The shortest way I can think of is by registering all HashableId model to your ServiceProvider before it automatically created by the model in repository.

class AppServiceProvider extends ServiceProvider
{
    public function register() 
    {
        // rest of the code
        // register model in the repository
        HashId::make((new SomeModel())->getHashKey(), 'customsalt');
        HashId::make((new AnotherModel())->getHashKey(), 'customsalt');
    }
}

this way, the HashId repository will be generating new instance before it get created automatically by the model, thus the model will always get the same instance from the given model.

but if you prefer doing this on the model, you can make workaround in this line, basically you have to create the hashid instance first with the custom salt.

[veelasky]

Cool, thanks. We're looking forward to it.

As input: one of the patterns often seen in laravel is the possibility to inject a closure like:

$generator = function(string $key) {
    substr($key, -10) . 'addSalt';
}

Veelasky\LaravelHashId\Repository::generateKeyUsing($generator);

As I think the Repository is registered as singleton, I think this should be fairly easy to do. But you might find a better approach.

yes it was valid approach, but the point of this package is to have different hash from different model, by using that method it will always generate the same id for given integer.

[kohlerdominik]

Cool, thanks. We're looking forward to it.
As input: one of the patterns often seen in laravel is the possibility to inject a closure like:

$generator = function(string $key) {
    substr($key, -10) . 'addSalt';
}

Veelasky\LaravelHashId\Repository::generateKeyUsing($generator);

As I think the Repository is registered as singleton, I think this should be fairly easy to do. But you might find a better approach.

yes it was valid approach, but the point of this package is to have different hash from different model, by using that method it will always generate the same id for given integer.

That's why in my example i passed the $key-variable, which should hold the models $hashKey, right? So it's either possible to ignore it, or to use it in the callback function.

[veelasky]

I have made some changes, see d55d33a

for the model to have custom salt implemented by any logic you just have to add a method getHashIdSalt() method to your model, so it will overwrite the repository using the custom salt, like the test I provided here.

let me see if it works, and I'll release it to a new minor version.

[veelasky]

Cool, thanks. We're looking forward to it.
As input: one of the patterns often seen in laravel is the possibility to inject a closure like:

$generator = function(string $key) {
    substr($key, -10) . 'addSalt';
}

Veelasky\LaravelHashId\Repository::generateKeyUsing($generator);

As I think the Repository is registered as singleton, I think this should be fairly easy to do. But you might find a better approach.

yes it was valid approach, but the point of this package is to have different hash from different model, by using that method it will always generate the same id for given integer.

That's why in my example i passed the $key-variable, which should hold the models $hashKey, right? So it's either possible to ignore it, or to use it in the callback function.

yes but if we want to be able to utilize custom salt in model(s), I have to change some signature on the repository and the contract and it will resulted a whole new different version.

[veelasky]

I have made some changes, see d55d33a

for the model to have custom salt implemented by any logic you just have to add a method getHashIdSalt() method to your model, so it will overwrite the repository using the custom salt, like the test I provided here.

let me see if it works, and I'll release it to a new minor version.

@kohlerdominik have you tried this one?

[kohlerdominik]

Hi

I just tried, but it didn't reproduce the hash from version 1 when using

    protected function getHashIdSalt()
    {
        return 'MySalt';
    }

    protected function getHashKey()
    {
        return substr(static::class, -4);
    }

For reference, how it looked like in version 1:

    protected function getHashKey()
    {
        return 'MySalt:' . substr(static::class, -4);
    }

[veelasky]

Hi

I just tried, but it didn't reproduce the hash from version 1 when using

    protected function getHashIdSalt()
    {
        return 'MySalt';
    }

    protected function getHashKey()
    {
        return substr(static::class, -4);
    }

For reference, how it looked like in version 1:

    protected function getHashKey()
    {
        return 'MySalt:' . substr(static::class, -4);
    }

in version 1, if you're not modifying makeHashedIdSalt() method, it should look like this, so your model should look like this

class SomeModel extend Model
{
    use HashableId;
    
    public function getHashIdSalt()
    {
        // retain salt from v1
        return substr(static::class, -4).substr(config('app.key', 'lara'), -4);
    }
}

you don't have to override getHashKey() in version 2, all you have to do is to provide the salt from previous version, the method getHashKey() merely get the hashid instance on the collection in the repository singleton.

[kohlerdominik]

Got it, thanks. Yes, tests succeed and resulting HashIDs are the same as in V1, so this change does the trick for me. Once you tagged a release, i'll update to v2. Thanks a lot!

One more question: it comes with a slight performance penalty, right? The original idea of using a HashIdRepository was, that not always a new instance of Hashids was required, I guess. Now, we force-create a new instance, assign it to the repository and then retrieve it with every use of a Hashid in the model, correct?
I think it's nearly the same as in V1, and we never noticed an issue with the performance there, so I guess it doesn't really matter. Just wondering if I understood correctly.

[veelasky]

Got it, thanks. Yes, tests succeed and resulting HashIDs are the same as in V1, so this change does the trick for me. Once you tagged a release, i'll update to v2. Thanks a lot!

One more question: it comes with a slight performance penalty, right? The original idea of using a HashIdRepository was, that not always a new instance of Hashids was required, I guess. Now, we force-create a new instance, assign it to the repository and then retrieve it with every use of a Hashid in the model, correct?
I think it's nearly the same as in V1, and we never noticed an issue with the performance there, so I guess it doesn't really matter. Just wondering if I understood correctly.

once again, you're correct... it does have the fallback to force-create a new instance. but as I said before, I'm trying to find a way without having regressing to refactor the code all over.

I'm planning not only supporting HashId but also ulid but until then, this will have to suffice,