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

Some criticism of this package #30

Open
uuf6429 opened this issue Apr 13, 2021 · 0 comments
Open

Some criticism of this package #30

uuf6429 opened this issue Apr 13, 2021 · 0 comments
Labels
enhancement New feature or request

Comments

@uuf6429
Copy link

uuf6429 commented Apr 13, 2021

First of all, apologies to disregard the template. I'd like to get straight to the point on a few issues.

  1. Expired records are not excluded by default, which isn't expected behaviour and causes end users to hack up their own way (this should be done in the apply method of the scope class).
  2. Regarding setExpiresAtAttribute:
    1. Setting the value of expires_at is very weird - there's a lot of funny parsing going on when instead it should have been a simple is_int($x) ? Carbon::now()->addSeconds($x) : $x - the current code somehow doesn't work for me in a simple test with timestamps and Carbon::setTestNow()
    2. If the dev named the column something else the setExpiresAtAttribute code will not be triggered for the other column name - this could be confusing/misleading
  3. Regarding the Expirable trait:
    1. It is missing method PHPDoc (it should document the macros from ExpiringScope).
    2. Instead of redeclaring @property $attributes and @property $dates in that trait, maybe use @mixin HasAttributes instead.
    3. Careful that even if getSeconds trait method is protected, it could come down to some nasty incompatibility with other traits or implementing class. Same situation with importing all the methods from InteractsWithTime trait.
  4. I'd consider better names for the macros, for example Subscription::withoutExpiration is more idiomatic than notExpiring.

On a different perspective, there's a competing package that solves some issues above but has its own problems.
Maybe consider partnering up to bring the best of both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant