Skip to content

Comments

Adds Encryptable model behavior#138

Merged
bennothommo merged 17 commits intowintercms:developfrom
der-On:patch-1
Sep 15, 2023
Merged

Adds Encryptable model behavior#138
bennothommo merged 17 commits intowintercms:developfrom
der-On:patch-1

Conversation

@der-On
Copy link
Contributor

@der-On der-On commented Jan 21, 2023

This pull requests adds the Encryptable model behavior which is based on the Encryptable model trait but can be added dynamically using the models static ::extend method.

Usage:

MyModelClass::extend(function ($model) {
  $model->addDynamicMethod('getEncryptableAttributes', function () {
    return ['private_attribute'];
  });
  $model->implement[] = \Winter\Storm\Database\Behaviors\Encryptable::class;
});

@what-the-diff
Copy link

what-the-diff bot commented Jan 21, 2023

  • Added a new file
  • The class Encryptable extends the ExtensionBase class from Winter\Storm\Extension namespace
  • In the constructor, we call bootEncryptable() method and pass $parent as an argument to it (which is actually our model)
  • We check if getEncryptableAttributes() exists in our model or not using methodExists(). If this function doesn't exist then throw exception with message "You must define a getEncrytableAttributes..."
  • Then we bind two events: beforeSetAttribute & beforeGetAttribute which are triggered when setting/getting attributes of models respectively

@mjauvin
Copy link
Member

mjauvin commented Jan 21, 2023

@der-On can you look into the failed tests?

@mjauvin mjauvin self-assigned this Jan 21, 2023
@mjauvin mjauvin added this to the v1.2.2 milestone Jan 21, 2023
@bennothommo
Copy link
Member

@der-On one thing I've noticed is that during the bootEncryptable() method, you're doing a check to see if the $encryptable property exists in the model, and throwing an exception if it doesn't exist. The problem is - your comment above stipulates that the $encryptable property should be protected (like it is with the Encryptable trait), but unfortunately this won't work in the context of a behavior, because they cannot access protected or private properties and methods.

Also, your example has the getEncryptableAttributes() method dynamically added, which would make the $encryptable property redundant.

So to make this work, either that property check needs to be dropped, or the $encryptable property needs to be made public. Ideally, it would be better DX if they don't have to create a dynamic method every time they wish to use a behavior, but on the flip-side, I'm not sure allowing the $encryptable property to be public is a good idea.

If my scoped and local extensions PR is merged, we'd have a mechanism to be able to allow us to access protected properties, so this could be a good push to get that merged.

I'll commit a test and my changes which demonstrate the behavior working when $encryptable is public, but ultimately we need to make a decision on how we want to go forward with that scenario first.

@der-On
Copy link
Contributor Author

der-On commented Jan 23, 2023

@LukeTowers @bennothommo wow thanks for all the input and improvements. This is becoming interesting.

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@LukeTowers
Copy link
Member

@bennothommo what's left on this?

@bennothommo
Copy link
Member

Given that the extension changes were merged, just converting these changes in this PR to use that instead.

@LukeTowers
Copy link
Member

@bennothommo is that something that you are able to take care of or are you wanting @der-On to do that or is @mjauvin interested in doing that (he's currently assigned to the PR?).

@bennothommo
Copy link
Member

If someone else can tackle it sooner, feel free, otherwise I'll circle to it sometime in the near future.

@LukeTowers LukeTowers removed this from the v1.2.3 milestone Jul 7, 2023
@LukeTowers LukeTowers added this to the v1.2.4 milestone Jul 7, 2023
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@der-On
Copy link
Contributor Author

der-On commented Sep 11, 2023

@mjauvin Made all the changes. Let's see what the tests say.

@der-On
Copy link
Contributor Author

der-On commented Sep 12, 2023

@mjauvin Seems like there is some unknown error with the code analysis but unit tests have passed.

@mjauvin
Copy link
Member

mjauvin commented Sep 12, 2023

@bennothommo how do we solve this with phpstan, we're accessing a protected property with the help of the extend() method...

@bennothommo
Copy link
Member

Surprised that the tests are passing - I think this branch is still missing all the changes to the Extension framework.

@bennothommo
Copy link
Member

Should be all good now.

@der-On der-On requested a review from mjauvin September 13, 2023 06:29
@der-On
Copy link
Contributor Author

der-On commented Sep 13, 2023

Awesome! Sorry for me taking so long to get back to this.

@mjauvin
Copy link
Member

mjauvin commented Sep 13, 2023

Looks good to me!

@bennothommo bennothommo merged commit de146af into wintercms:develop Sep 15, 2023
@der-On
Copy link
Contributor Author

der-On commented Sep 15, 2023

Whohoo! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants