Skip to content
This repository was archived by the owner on Apr 7, 2021. It is now read-only.

Add support for Laravel 5.7 #28

Closed
axelitus opened this issue Sep 6, 2018 · 7 comments
Closed

Add support for Laravel 5.7 #28

axelitus opened this issue Sep 6, 2018 · 7 comments

Comments

@axelitus
Copy link

axelitus commented Sep 6, 2018

Currently this package cannot be installed with Laravel 5.7 due to the restriction:

"require": {
    "laravel/framework": "5.5.*|5.6.*",
}

Please add support for Laravel 5.7

@austinheap
Copy link
Owner

Feedback please: 573112a. 😁

@axelitus
Copy link
Author

I'm testing this right now. I have already commented an issue in the README file.

@austinheap
Copy link
Owner

@axelitus Good catch! Fixed in commit: 33db743

@axelitus
Copy link
Author

axelitus commented Sep 17, 2018

I found an issue with the commit 573112a, the encrypted fields are being correctly generated but an unencrypted value is being stored in the database.

I've debugged the code and the issue is related to this change introduced 21 days ago (which in fact breaks support for Laravel 5.6 also).

Previously, on insert the $this->attributes property was used directly, now $this->getAtttributes() method is called, which in fact is replaced by the HasEncryptedAttributes trait and this line gets executed which unencrypts all attribute values before insert.

The result is that the package "is broken" because it doesn't achieve it's purpose.

My proposal is to change the getAttributes() method to getUnencryptedAttributes() and use Laravel's Illuminate\Database\Eloquent\Concerns\HasAttributes::getAttributes() implementation directly, but I'm not sure if the getAttributes() is used elsewhere in your code that needs to be changed.

@austinheap
Copy link
Owner

austinheap commented Sep 17, 2018

@axelitus Can you make a separate issue for this and include code to reproduce the bug? Without digging in too much, this seems accurate and would explain other oddities when using 5.6/5.7.

@axelitus
Copy link
Author

I'll open an issue later on with a use case

@austinheap
Copy link
Owner

Closing in lieu of #30.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants