[V6] [BC] Return string on getPermissionClass(), getRoleClass()

[erikn69]

This PR is for testing discussion #2367, this could be a breaking change if someone uses getPermissionClass(), getRoleClass()

Tests with this change
-> Time: 00:09.050, Memory: 46.00 MB
Test without this change
-> Time: 00:09.427, Memory: 48.00 MB
Seems like this change uses less memory, also I think that this uses less cycles because it is not calling app() helper on every instance, and avoid making contract instances on every time(Tested on PHP 8.1)

[drbyte]

Thanks for this.

Need to research the use-cases where Issues were logged and PRs contributed to add those Get methods, to anticipate what will break by userland code having expectation of them returning an instance instead of a class-name string.

[erikn69]

@drbyte here 37f3178, tag v2.16.0, PR #746

[drbyte]

@lk77 will this PR cause problems to your custom models?

[lk77]

@drbyte i don't think so, we don't have that use case anymore, but i think it's something that can be useful to someone else.

those who have a custom model will need to make the necessary changes, like accessing the model using $this->permissionClass:: , in all the overriden methods that make use of the models.
For me it's perfectly fine on a major release.

The instance won't be stored which will result in less memory usage.

i pulled the pr branch and everything was working fine, the custom model did get called, and i didn't saw any fallback to the Permission::class

Using the app helper to retrieve a model instance do not really make sense, they are meant to be statically called, i think at the time i kept the app() calls and only made the parameter dynamic, but getting rid of the app helper and using static calls is way better, and i think it can potentially better solve the initial issue, having an instance stored on a property can be less reliable than statically calling a FQCN

[drbyte]

@erikn69 I'm good with this, unless you feel there's more to explore with it before merging.

Do you want to rebase it onto latest main (where test suite was refactored a bit) and update the tests' namespace from Test to Tests, before merging?
(Or I can do that after merging, either is fine.)

[erikn69]

you feel there's more to explore with it before merging

I think it's the right way to go, and it's ok for a major release,
I already tried this in my APPs and nothing fails me, despite the fact that I have some customizations, but we have to add a note to the upgrade guide

REBASED

[drbyte]

we have to add a note to the upgrade guide

Agreed.