Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make classes final #316

Open
wants to merge 6 commits into
base: 2.x
Choose a base branch
from
Open

Conversation

jrushlow
Copy link
Collaborator

@jrushlow jrushlow commented Apr 26, 2024

  • replaces the annotated @final with PHP final keyword. Which in turn cleans up our tests a wee-bit..

As a side effect of that, we need to be able to mock ResetPasswordRandomGenerator, ResetPasswordTokenGenerator, & ResetPasswordCleaner. Which you cannot do when using the final keyword.

One work around would be to use something like https://github.com/dg/bypass-finals. This approach is cool, but it's more or less a "one size fits all" approach. Part of our tests guarantee that we are not using final classes where we shouldnt. So instead of "flaky" tests, I went with...

Implementing interface's for the classes we need to mock. I'm usually the first to say NO when it comes to adding to the public API that will increase our maintenance burden / technical debt. But, I think having these interfaces will be beneficial in the long run.

a) We sort of tease that hey, you can create your own ResetPasswordHelper by implementing the ResetPasswordHelperInterface. But as you dig deeper down the rabbit hole, you quickly find out that you can't actually do that. Mostly because our generators, models, and utils are final xor internal (As they should be). Swapping out the concrete argument and return types with the interfaces allows developers to truly implement their own customized implementations without needing us to change our own implementations in the bundle.

and

b) should there be better, more secure, less problematic, etc. methods of doing say, generateToken(), providing an interface will take some of the pressure off of us to push out a fix while trying to preserve BC. Atleast, thats the goal.

Todo:

  • update tests that mock these classes
  • update this PR text to include the interfaces that we're adding (assuming we press forward with this)
  • document changes in the upgrade log (I'm sure someone out there is using these in userland) 👀
  • flesh out the new interfaces

@jrushlow jrushlow added the Minor Minor Enhancement label Apr 26, 2024
@jrushlow jrushlow added this to the 2.x - Pre-release milestone Apr 26, 2024
@jrushlow jrushlow mentioned this pull request Apr 26, 2024
20 tasks
@jrushlow jrushlow added Feature New Feature and removed Minor Minor Enhancement labels May 17, 2024
@jrushlow jrushlow force-pushed the feature/finals branch 2 times, most recently from a7630f4 to 2b45351 Compare June 6, 2024 15:10
ResetPasswordTokenComponents is internal, consumers now can
create their own implementation of the ResetPasswordTokenGenerator
without having to use our internal token components class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant