hasPermissionTo never checks roles (because HasRoles trait does not work with Collection)
top-master opened this issue · 44 comments
laravel-permission/src/Traits/HasRoles.php
Line 217 in 0c49776
Above may be a bug, because Laravel's query results to collection, not an array.
I mean, Currently hasPermissionTo is just an alias for hasDirectPermission,
because hasPermissionViaRole always returns false.
And that, because $permission->roles returns Collection, never raw-array.
hasPermissionTo never checks roles (because HasRoles trait does not work with Collection)
I think that you are wrong, if $roles is not an array it continues to collection handler
laravel-permission/src/Traits/HasRoles.php
Line 227 in 0c49776
because $permission->roles returns Collection, never raw-array.
That is not the only call for that method, also there is many tests for hasRoles
https://github.com/spatie/laravel-permission/blob/main/tests/HasRolesTest.php
because hasPermissionViaRole always returns false.
Not, sure you break something
I openned issue without reading that last line, which you call "Collection handler".
Basically, instead of above issue, I should open 2 new issues.
First issue
It simply uses $this->roles, even if "roles" relation is never loaded.
Instead, it should throw an exception, like:
"roles relation not loaded"
Basically, just returning false is a lie in this case.
Second issue
It does ->intersect without checking if $roles is instanceof Collection.
Which basically was hidding it from my review, I mean, you're lucky I don't review your code,
I would reject that instantly, because instanceof Collection check is missing.
After debugging for few hours
Many hours of debugging can save you 5 minutes of reading documentation
It simply uses $this->roles, even if "roles" relation is never loaded.
roles are always loaded with $this->loadMissing('roles');
laravel-permission/src/Traits/HasRoles.php
Lines 190 to 192 in 0c49776
Which basically was hidding it from my review, I mean, you're lucky I don't review your code,
I would reject that instantly, because instanceof Collection check is missing.
😄😄😄😄 Just because you can't understand how things work doesn't mean they're badly done.
As you say(before you deleted), this package is so many years old, it is because it is very tested(11k stars, sure you didn't give one)
Also, why do you delete your comments and their changes history, shame on nonsense?
"can't understand ... doesn't mean they're badly done"
Human-readability is important, you're basically saying:
- readers are forced to understand,
- without any
instanceofcheck, - and even without any comment.
But there are multiple other things badly done in that line, beside not being human-readable:
- if we pass to that method a custom class,
- which has a custom
intersectmethod, - said method is called, and maybe drops all tables, or something like that.
- it's a custom method, and it's allowed to do whatever it wants.
- it's your fault for calling that method without checking ;-)
"... it is very tested (11k stars, sure you didn't give one)"
- I gave one star ;-)
- Starts don't prove something is 100% well done,
- They're badly done wherever they're not well done.
It simply uses $this->roles, even if "roles" relation is never loaded.
roles are always loaded with $this->loadMissing('roles');
Hello, if you think there is a bug, do a failing test to prove it, also feel free to make a PR to make the code better
from my point of view the problem is that you can't understand the code(apparently you are the only one), because there are many tests on this package
https://github.com/spatie/laravel-permission/tree/main/tests
Finally there it is, the old saying of "we accept pull requests", which's basically just dodging work.
Also, you seem to point again and again to:
$this->loadMissing('roles');
Well, that line does not even exist, for example, on version 3 and 4 branches,
I will try updating to version 5 (current latest) once possible ;-)
However, that is history by now, what about the second issue, don't try to change the subject.
"you can't understand"
Wrong, re-read my comment, again and again until you understand, lol.
How much is even your StackOverflow reputation? I mean, please don't spam this thread until you have at least same:
7000
Well, that line does not even exist, for example, on version 3 and 4 branches,
I will try updating to version 5 (current latest) once possible ;-)
So much reputation and it seems that you have to explain the basics, first when you start a new issue report you must specify the package's version, laravel's version and PHP version, otherwise everyone will assume that you are in the current versions,
For example, several here have reviewed what you say, and apparently you use an outdated version of the package and everything you report has probably already been fixed according to the tests, you only wasted the time of people with the intention of helping
Second, old versions reaches they end of life and they are without support, current is 5, and in a short time they will go to 6
You also arrive with your attitude that "everything is badly done and full of bugs" (without showing any failing tests), and "that I have a super reputation and I'm better than everyone"
stackoverflow reputation don't prove something here, better show how many PHP contributions you have made to open source in this month at least, stackoverflow mostly it is to answer basic questions of the noobs
please don't spam this thread until the day you beat my reputation
😄😄😄😄😄 This is a public thread, It is also necessary to explain how this works
the old saying of "we accept pull requests", which's basically just dodging work.
This is open source, it is not a paid job where you order something and everyone must put to work to please you, I am not the author nor a member of the staff, I only review issues in my spare time to help, and this is not an bug
Open source software is developed in a decentralized and collaborative way, relying on peer review and community production.
That means that if you can improve something you can collaborate instead of complaining and belittling the work of others
Wrong, re-read #2332 (comment)
Ok
readers are forced to understand,
Not, easy to understantd to me, do not generalize with your personal appreciations, but of course depending on the level of knowledge or intellectual capacity something is difficult or easy to understand, when I read a laravel package for the first time it seemed difficult to understand, now I understand it with a single glance
without any instanceof check,
If all supported checks passed, the main one supported remains by elimination(I didn't write that code but I got it the first time), It's called early returns, read about it, also if you check instanceof Collection, still might not be a collection of roles, human error can be more extensive and you won't finish the checks, you will only increase the cost of performance
and even without any comment.
no need to comment on the obvious things, but i gonna point you the comment with the supported datatypes
laravel-permission/src/Traits/HasRoles.php
Lines 186 to 190 in 490fa00
if we pass to that method a custom class
it's a custom method, and it's allowed to do whatever it wants.
I'm using custom classes and everything works, but when using custom classes you must prevent any incompatibility by your own responsibility, it is something that everyone knows, no package can predict all possible customizations, extendings, and others
which has a custom intersect method,
intersect comes from laravel collection, I doubt someone will change that utility but if it does, read the previous point again, but when they drop PHP 7 support for sure they will do typed methods, for now it seems that no one has come up with the idea of passing unsupported datatypes(yes, it could be better but if the community needs that change, a PR has already been done)
said method is called, and maybe drops all tables, or something like that
lol, in 15 years I have never seen anything like this happen in any framework, If someone is capable of doing that, it is because they are doing their job very badly, normally when you do some customization you also do unit tests to avoid unexpected behaviors
it's your fault for calling that method without checking
Tested, it's not a bug, if you think there is a bug, do a failing test and surely someone would take you seriously
don't try to change the subject.
You can't order that, authors can modify what they want in their repository without your permission or approval.
Well, that line does not even exist, for example, on version 3 and 4 branches,
I will try updating to version 5 (current latest) once possible ;-)
This information is very important before opening issues, everyone's time is valuable
well, I think I have answered all the questions, bye
"stackoverflow mostly it is to answer basic questions of the noobs"
@parallels999 If you can't even answer a noob's question, what does that make you - right, even worst noob ;-)
Secondly, there are many pro questions, go answer those.
"So much reputation ... specify the package's version"
First of all, this thread is about current-latest version, hence no need to specify version,
I mean, the version is only related to the first of the issues which I said I should open,
all other issues are about current-latest version.
Secondly, I did not even open such issue(s) yet, hence you wasted your and my time by answering here instead of waiting for said issues to be openned.
Lastly, learn english.
@PaolaRuby I hope no one wastes time reading a spammer's excuses, most of which did not make sense.
Of course, I meant inline-comment, the doc-comment does not explain why instanceof check is missing (and should never, we have inline-comment for that).
Okay, let's close this, only excuses here ;-)
my comment explains enough, all know these commenters are just spamming excuses.
If you can't even answer a noob's question, what does that make you? right, even worst noob ;-)
If you can't understand three simple lines of code, what does that make you? right, even worst noob ;-)
Lastly, learn english.
Yes, I'm learning at nights, no one is born knowing a different language, I don't know if you say it for lack of arguments or simply some kind of racism on your wall
First of all, this thread is about current-latest version, hence no need to specify version,
I will try updating to version 5 (current latest) once possible ;-)
you contradict yourself apparently you are in a previous version
Lastly, learn coding.
Okay, let's colse this
Colse? Maybe I don't know much English, but that's not even a word
"the version is only related to the first of the issues which I said I should open,
all other issues are about current-latest version."
Can not even read?
"If you can't understand ... noob"
I already answered such bad excuses above: comment-1436704055
Go read that again and again, until you understand.
If all supported checks passed, the main one supported remains by elimination, It's called early returns, read about it, also if you check
instanceof Collection, still might not be a collection of roles, human error can be more extensive and you won't finish the checks, you will only increase the cost of performance
Can not even read? Your question was already answered by an contributor
Also you edit everything every moment, erasing all the history of the crazy things you have written
I read that already, and no, 1 micro second performance is no excuse for badly done code.
I read that already, and no, 1 micro second performance is no excuse for badly done code.
Still, that is not a bug
Make a failing test, that would prove you right and the members would correct it.
That you have made a bad implementation is not a problem of the package
"is not a problem of the package"
The package's function accepts multiple types, and checks most, except Collection.
No need for me to explain how to unit-test that obvious mistake, but try something like:
class MyHasRolesTest extends TestCase
{
function it_should_be_type_safe_for_collection()
{
$this->assertFalse($this->testUser->hasRole(new class {
public function intersect()
{
Assert::fail('Should only be called on Collection.');
}
}));
}
}
Use markdown correctly, ```php
laravel-permission/src/Traits/HasRoles.php
Line 186 in 6e0f957
Sending unexpected datatypes would be your bug, not the package's bug
Assumption, it is as if you did
\App\Models\MyModel::create([
[
'field'=>'value1'
],
[
'field'=>'value2'
]
]);On that example, laravel just create one empty db row without any exception, because it receives an array, but doesn't check that it's not an array of arrays(unexpected value)
Go report that to Laravel, others making same mistake is no excuse ;-)
Also, at least Laravel does nothing with the new row, but you call that wrong instance's method.
"Use markdown correctly, ```php"
Good joke, or good spam.
Just because that's what you do; we report issue and you spam excuses.
Here:
See above image, people can read that, and you complain about coloring.
at least Laravel does nothing
laravel just create one empty db row without any exception
Lol, you didn't read, laravel creates an invalid row
Go report that to Laravel, others making same mistake is no excuse
Exactly, others making mistakes is no excuse, you mistakes are not bugs
Good joke, or good spam.
No one has spammed you, check the meaning of spam, so far everyone has answered your questions, despite your behavior
"Use markdown correctly,
```php
Apparently you don't know how markdown works, I'll show you
if you use it correctly you can use hightlight, and that does help the readability
```php
array(
'test' => 'test'
)```js
{
test : "test"
}here not readability
Lol, looks like you are the only spammer here
But you maybe you need that, that could help you understand how the source code works.
You still have a lot to learn and a lot to grow up.
"answered your questions, despite your behavior"
Right back to you, there was no question,
only me pointing out an issue, and you spamming excuses.
Despite your behavior, this was fun.
@drbyte @freekmurze spam here https://stackoverflow.com/users/8740349/top-master
Here:
Despite your behavior, this was fun.
It's just that you deleted all the crazy things you wrote, that no one who doesn't equal your stackoverflow reputation should answer you,
please don't spam this thread until the day you beat my reputation
don't try to change the subject
this was really fun, from your behavior I imagine that you are between 15 and 18 years old
It's not removed, it's not crazy, it's just edited:
"don't spam this thread until you have at least same:"
See above image, people can read that, and you complain about coloring.
Human-readability is important
Yes, you are complaing about human-readability when people can read and understand everything except you
"Use markdown correctly, ```php"
Good joke, or good spam.
It seems that you only complain about readability when it suits you, or you just can't manage that you didn't know how to use markdown
It's not removed, it's not crazy, it's just edited
history removed, I imagine it must be very embarrassing for someone to read what you wrote
"only complain about readability when it suits"
That image is not code, but if it was, it would contain instanceof check, unlike your gibberish, lol.
Yes, that's called edit, go complain to GitHub if you want the old version.
Also, look who is talking:
history removed, I imagine it must be very embarrassing for someone to read what you wrote
Still
Spammer.
@top-master if you don't like this package, there is other alternatives, no one forces you to use this package, and yes, grow up, both
Lines 92 to 97 in 490fa00
I like the package, I don't like excuses and/or spams.
keep the package as simple as it can be
Freek Van der Herten
No one has spammed you or given you excuses, most of the responses you received from sporadic collaborators are correct
They're badly done
This is software from Spatie, reviewed by Freek Van der Herten, whose reputation is comparable to Taylor's. He has created many plugins for Laravel and is one of his regular collaborators. Any "bad code" complaints with the actual author, not the contributors
This is a belgium package for everyone, not all speak english, it is used as a standard to facilitate communication.
finally there it is, the old saying of "we accept pull requests", which's basically just dodging work.
Everyone is invited to collaborate, the nationality or reputation of any page is not taken into account for the reviews, answers, or issues reports
no one is "dodging work", if that was the case you wouldn't have even gotten all your questions answered
To avoid incorrect answers or confusion, specify all the information required in ISSUE_TEMPLATE/bug_report.md
Have a nice day
"all your questions answered"
Wrong, most of if not all the responses were excuses and/or spams,
as proof, a simple search for "?" sign, and what you find in my comments is only:
-
Firstly, my response to a spammer:
"How much is even your StackOverflow reputation?"
-
Secondly, my other response to some other spammer who tried twisting my words:
"Can not even read?"
It's common sense that as long as there was no question, there is no answer either, only excuses.
Basically, your entire comments come down to:
- We have doc-comment.
- We don't need to check input type, which has 1 micro second performance penality.
- Whatever happens, our doc-comment means it's your fault, check yourselves each time you call the function/method.
My answer:
- PHP ignores your doc-comments, and is not type-safe in this case.
- Each such mixed input function/method is responsible to check it's input types.
- Don't make bad excuses, it's your fault, because both PHP and most IDEs silently ignore doc-comments.
- Here, even a unit-test.
Don't make bad excuses, it's your fault
Why would it be my fault? i am not the author of the package, send your complaints to Spatie members and grow up
and is not type-safe in this case
It still can't be because it maintains compatibility with old versions of laravel and php
Firstly, my response to a spammer:
"How much is even your StackOverflow reputation?"
You have no contribution to any spatie package on your profile, to me you are the spammer here
"send your complaints to Spatie members"
@angeljqv I just did, no one asked for your spam, no one tagged you, this is for them to read.
well, it is a public thread on a public repo, grow up and stop complaining about the free work you receive from spatie
"grow up, both"
@angeljqv Look who is talking, take your own advice, and stop spamming.
I think that github needs an option to block users, I have many unwanted notifications from a spammer kid 😄😄😄😄
Ignore all "spammers kids" as they call themselves in recent edit, and as author,
I want at least below to be seen by members:
Everyone else, learn how to unfollow thread,
and remember the good old "Silence is golden." comment in empty index.php files.
Check these contributors' accounts, we see almost zero commitment to anything, but yet they are pretty active on spatie packages only.
Maybe those are fake accounts created by members,
which allows them to comment whatever they want, without damaging their reputation.
Also, the number of contributors which GitHub shows increases, which makes the package look more active.
Unfortunately, seems GitHub has not any check present to prevent said matter, and even reporting them didn't do anything.
remember the good old "Silence is golden."
"no one tagged you"
"to be seen by members"
"Everyone else ... Silence is golden."