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

Bugfix/eloquent visibility not used #16

Merged

Conversation

TheM1984
Copy link
Contributor

@TheM1984 TheM1984 commented Aug 2, 2023

Summary

Currently if you would use the HiddenAttributes feature of Eloquent, the enums would still be visible.

This is because they are added after Eloquent hides attributes if needed.

With this change I executed the function to hide the attributes after the AddEnumAttributes was performed.

Test Report

❯ ./vendor/phpunit/phpunit/phpunit
PHPUnit 10.2.7 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.8
Configuration: ./enum-eloquent/phpunit.xml

...........................                                       27 / 27 (100%)

Time: 00:00.076, Memory: 14.00 MB

OK (27 tests, 58 assertions)

StyleCI

As far as I can see everything should be good however, I was not able to run the StyleCI test myself.

Edit: saw that the build actions for the PR has a pass for StyleCI

New Functionality Tests

  • In case you add new functionality, it must be covered by tests

@fulopattila122
Copy link
Collaborator

Nice catch, nice solution, thank you ✌️

I'll merge and release it next week, once I'm back from vacation.

@TheM1984
Copy link
Contributor Author

Hey, hope you had a good holiday, and the post-vacation workload is not too much.

@TheM1984
Copy link
Contributor Author

Hey @fulopattila122,

Just a quick note to remind you about this open PR.
If there's anything I can do to facilitate the review process, please feel free to reach out.

Thank you for your time and consideration!
Mark

Copy link
Collaborator

@fulopattila122 fulopattila122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR and the pinging; sorry that it took this long

Changelog.md Outdated Show resolved Hide resolved
@fulopattila122 fulopattila122 merged commit 029052e into artkonekt:master Sep 25, 2023
1 check passed
@fulopattila122
Copy link
Collaborator

It's not working with Laravel 8: https://github.com/artkonekt/enum-eloquent/actions/runs/6298264476/job/17096781736

I try to fix it, but if it takes too long, I'll just simply drop Laravel 8 support beginning with this release on

@fulopattila122
Copy link
Collaborator

I've found the exact minimum version that lets the tests pass, and it is Laravel v8.75.0. I presume that the fix is actually this one: laravel/framework#39880

The respective PR is a test-related fix, so most probably, the code would work outside of the test environment even with Laravel v8.22 - v8.74, but I don't have the bandwidth to prove it, so the next release of this package changes the required minimal Laravel version from 8.22 to 8.75.

Since Laravel 8 is EOL anyway, and upgrading to v8.83 shouldn't be a problem when a project is stuck with Laravel 8, I believe the number of people this change will negatively affect is very close to zero.

fulopattila122 added a commit that referenced this pull request Sep 25, 2023
- Releases #16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants