[7.x] Changed fixed value, used laravel string helper function

[pathak-vikash]

This PR is about remove fixed counter ( like 7 for 'base64:') and used laravel Str:after() helper function to improve code readability.

Edited:

This PR is based on #31069

[jmarcher]

I found that most of the changes made the code less readable.

[Miguel-Serejo]

Is this necessary? At most I could see a comment being added to clarify what the numbers mean, but they're pretty self-explanatory in most cases.

I also don't like the introduction of the $prefix variables. The code would be more readable as Str::after($key, 'base64:')in my opinion, as these are fixed values.

[pathak-vikash]

Hi @jmarcher & @36864 the code would be more readable. As let's take an example

// Before
if (Str::startsWith($header, 'Bearer ')) {
    return Str::substr($header, 7);
}

In the example above, it doesn't make sense to keep the value as fixed: 7. Now the change in this PR like:

// After
if (Str::startsWith($header, $prefix = 'Bearer ')) {
    return Str::after($header, $prefix);
}

Also defining the variable $prefix to avoid duplicate checks with string token and make more sense to read as The value after this prefix.

@jmarcher I'm open to update this if you feels any portion of code you feels not readable. Please point me there I'll update.

FYI, the requested changes based on the previous #31069

[Miguel-Serejo]

My concern here is that we're hindering performance for a subjective increase in readability that could be achieved with a simple comment.

Yes, it's a minor performance hit, but it's also a minor increase in readability.
I don't think anyone looking at this could would be confused as to what substr($key, 7) does in the context of the previous if (Str::startsWith($key, 'base64:')) { statement.

The one point where a change would be useful, in my opinion, is in the dynamicWhere function, but the same could be accomplished with the following:

// trim leading "where" from method name
$finder = substr($method, 5); 

As for the referenced PR, I also disagree with the changes on the same terms, but wasn't around to voice my opinions before it got merged. That one is even worse because the encryption provider is registered on every request, while the files changed in this PR are less used.

Also for some additional context, I offer #30960 which was merged as a strict micro-optimization, arguably at the cost of readability according to some people.

[pathak-vikash]

@36864 I'm checking this for 10k requests and it's producing me the similar result for both of the case. There would be some very minnor impact for this change... but not as much as in terms of readability it may acceptable.

[taylorotwell]

If nothing is broken I think it's fine.