[12.x] Split str class into logically grouped separate classes #52979
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
shaedrich
changed the title
| use Ramsey\Uuid\UuidFactory; | ||
| use Symfony\Component\Uid\Ulid; | ||
|
|
||
| class Generator |
There was a problem hiding this comment.
Maybe another name for example StringGenerator
There was a problem hiding this comment.
Not bad, let's see what the others say 👍🏻
There was a problem hiding this comment.
What do you think of \Illuminate\Support\Str\Generator?
There was a problem hiding this comment.
Uhmm that could be a solution but then the other classes also needs to be added to the folder Str
There was a problem hiding this comment.
Yep, that would be the idea then 👍🏻
|
This seems like a pain to use, I need to remember where all the functions are in order to use them. As a Developer you use the class and I do not care about how long that class is. The problem you present can be solved with splitting the methods in different Traits an use them directly in the Str class. |
|
@jmarcher The whole framework consists of more than this single class—how do you remember these? While traits might be part of a solution, this isn't taking it far enough in my opinion. It's not "just too long", it's muddles the domain of the class into a meaningless class, that does something. Other classes in Laravel do have meaning, a domain, and a scope. This class, however, is treated like a dumpster, everyone just throws their stuff in mind- and carelessly, because it's there conveniently. |
I think this can be resolved by settings/ using these as the standard so developers kan still use Str::mixin(new Casing());
Str::mixin(new Generator());
Str::mixin(new PatternMatcher());
Str::mixin(new Replacer());
Str::mixin(new Sanitzer());
Str::mixin(new StrGrammar());Like these in the example and I think this is a really good solution/ idea to split the code up. Like @shaedrich mentiod |
|
Is it possible, that the |
Whenever I want to use a Str method I go to the class and read the methods included (with traits) whenever I want to use the response class I do the same... Really I don't get exactly what this is trying to solve is it because in your opinion the class is too long? Then propose separation into traits |
|
Woah, you really are hung up on on it being too long, despite me having explained already that it is more than just "too long". Please (re)read my previous comment concerning that very question. |
This is because this PR changes 160 files, and almost 3k lines of code and also big breaking change for all the existing Laravel apps, and the gain from it is none, just more things to remember. You have to deal with it in open-source people have different opinions, I mean this may get merged and I will need to use it, and then deal with this change, this is one core idea of open-source, meaning to gather different opinions and build the best solution out of it. |
|
I can see why this would be useful. Regarding the breaking change: Instead of throwing this out the window at once, it could be slowely faced out (deprecated), if we would put the It is probably a good idea to have the namespace This change could also be a good orientation for some other groupings of present/future code, if needed. |
|
@taylorotwell |
|
Maybe make a new PR with just the changes because changes this at once is alot of review work |
I wouldn't recommend that as this PR creates a major breaking change to Laravel Framework and any PHP library relying on |
With Yes, it is a breaking change (hence the 12.x tagging) and a lot of files were (briefly) touched, but as both @func0der and @flavio-schoute both have pointed out, it is extremely(!) easy to opt out of this change individually, even after a potential phase out in 13.x, as @func0der suggested.
The changed files won't become fewer and the majority has already vehemently positioned itself against this change, so I don't see this going through either way 🤷🏻 Thank you and @func0der nonetheless for your suggestions :) |
That's only for Laravel application, what about 32k packages relying on |
|
@shaedrich You can still have a partial win if you submit a PR which splits it using But something which creates backwards compatibility issues is starting from some major cons and needs especially strong functional pros to outweigh these. |
|
@Sophist-UK As I explained earlier, this is not a viable solution as it doesn't change the root cause of the problem. Maybe, we have maneuvered ourselves in a corner there where there is just no way out 🤷 Furthermore, it's always "just yet another try" and then, the PR gets rejected again for whatever reason, Taylor again doesn't disclose and gets everyone guessing, yet nobody knowing for sure except Taylor. And, what I hate the most: Everyone has expressed their opinion as to why this won't work, yet nobody acknowledged the current state being problematic. I can see why it is not easy to change it now, but that doesn't mean, it wouldn't be right to do so if we could. |
|
@shaedrich in your description above you said:
So this actually is trying to achieve two things:
The problem with 1. is backwards compatibility, and IMO that means it needs to be opt-out rather than opt-in. And given that macros or mixins are done at a class-level, this can be VERY difficult. The other issues with opt-ins or opt-outs for the main application is that we need to be VERY careful not to create backwards incompatibility with other packages that happen to use the full set of So, here is a suggested approach that you might be able to use to achieve your goals. Do the same separation as you have done above, but separate the core namespace Illuminate\Support;
class Str extends StrBase
{
use StrCasing;
use StrGenerators;
use StrPatterns;
use StrReplacers;
use StrSanitzers;
use StrPlurals;
}What a user who doesn't want the bloat can then do is easily to create and use their own version of the namespace MyApp;
class MyStr extends StrBase
{
use StrSanitzers;
}A couple of potential gotchas... Type matchingBecause you are not extending the Because packages may return Over time, as the use of Aside: This is where purists will say that Laravel should separate out Objects and Interfaces - and you should use Interfaces as parameter and return types, and use Objects that implement those interfaces as data. I have no idea why Laravel doesn't do this, but I think it likely that Taylor has a reason. That said, once this PR is accepted, there is nothing to stop you submitting a further PR to define interfaces for Naming of Abstract Classes and InterfacesIf you are going to use Abstract Classes or Interfaces then you probably need to follow https://www.php-fig.org/bylaws/psr-naming-conventions/ i.e. namespace Illuminate\Support;
class Str extends AbstractStrBase implements StrBaseInterface, StrCasingInterface, StrGeneratorsInterface, StrPatternsInterface, StrReplacersInterface, StrSanitizersInterface, StrPluralsInterface
{
use StrCasingTrait;
use StrGeneratorsTrait;
use StrPatternsTrait;
use StrReplacersTrait;
use StrSanitzersTrait;
use StrPluralsTrait;
}However, Laravel does not use this naming convention e.g. for Traits, and I think that you can see why Laravel doesn't widely use Interfaces in this way also. So I suggest that you don't use these in the next PR. I hope this helps you to find a way to achieve your objectives. |
This describes perfectly how all this works: Taylor has plenty of reasons for plenty of things. None of these, he discloses. So everyone thinks, they know what these reasons are, so they keep telling everyone their interpretations and tell them to open PRs based on these interpretations. And then—surprise!—Taylor closes the PR, because they are not his reasons, but doesn't bother to share his reasons. Who doesn't like guessing games?
What I explained above leads to people having confidence in one solution and one solution only: Their own, based on their interpretation of the words Taylor didn't bother to put into a reply. I'd love to believe you and share your dream, but even if I don't like it, I tend to subscribe to the stark contrast of the visions of @jmarcher and @crynobone. They tell the same story but with the polar opposite as outcome. They can't both become reality. And that's the problem: Taylor keeps us guessing and we all think, we know best, yet Taylor is the person who approves or rejects PRs, so us guessing is like playing the lottery. And getting one PR approved doesn't say, the next one does as well. If Taylor would agree to a roadmap, that would convey security for building a solution that is actually wanted at all, not solely at the mercy Taylor's current mood. I don't like being convinced to do a solution, that might change things for the better, if it was part of multiple changes, but in the end, the subsequent changes never get approved and therefore it changes nothing because this was just meant to satisfy me short term, so that when I make demands, I can be told that I already got my changes approved. But without clear communication from Taylor, this just isn't possible. All criticism aside: I do like your suggested solution as outlined in your last comment. And if the whole process wasn't the way it is and wouldn't only lead to frustration, I might actually try it. Thanks for your dedication to lay it all out. |
|
Well, I have also submitted a PR and had it closed without any explanation. And the scuttlebutt view of Taylor playing favourites with a selected few is worse than what you have suggested above. But whilst both of us might prefer a greater degree of explanation and transparency, 1) IMO it is better to have a decisive leader who takes definite decisions in a reasonable time-scale over a wishy-washy one who lets PRs languish for years; 2) In the end it is Taylor's baby, so he can adopt whatever leadership style he likes; and 3) We always have the option of forking Laravel and creating our own version which is identical except in one small way - and then have the onus of maintaining and supporting it as more and more commits need to be tweaked to make them merge; and 4) This is the reality, so we either deal with it or go away and use a different framework. Even if Taylor didn't say it explicitly, in this case there seems to be a consensus that your PR was not a good one because of the backwards compatibility issues, and (whilst there may be other reasons) this is a pretty good one to be going on with. That said, even with my approach which has backwards compatibility, I am not sure that Taylor would see that the benefits of a slightly smaller object would outweigh the additional complexity and the confusion of having yet another set of choices and decisions, a range of new errors for those that do create a cut-down MyStr and then have difficulties, and additional support and maintenance workload that will result from such situations. So - it is clearly up to you whether you want to make the effort to submit a new PR which doesn't have backwards compatibility issues - and which will therefore have a better chance of success or not bother. All of us who have been here understand the frustrations, and will not think bad of you if you decide not to take the risk of throwing good time after bad. All that said, I don't believe that splitting the code out in the way I suggested will take more than 20 minutes of your time. There might be a few tweaks to tests to make them work with the internals - but test failures should IMO be seen as a failure of backwards compatibility. Then you have to see what happens if you call a Str method on MyStr or have a Str parameter and feed it a MyStr object or vice versa, and write some code to give a better exception messages, to coerce data from Str to MyStr and vice versa, and (if possible) to help php to use the coercion methods automatically when needed - and to write additional tests all this new code. So, its probably a days work all in to create a new PR and see what happens. |
7 participants
As laid out in #47044 (comment), the
Strclass has become the jack of all trades device (in other words "bloated" and "haphazard"), having 1994(!) lines of code, which will be reduced to 939 with this PR, giving the class a slimmed down look. This PR groups the methods more logically into:New classes, their meaning and methods
createRandomStringsUsingSequence(),createRandomStringsUsingSequence(), andcreateRandomStringsNormally())uuid7(),orderedUuid(),createUuidsUsing(),createUuidsUsingSequence(),freezeUuids(), andcreateUuidsNormally())orderedUlid(),createUlidsUsing(),createUlidsUsingSequence(),freezeUlids(), andcreateUlidsNormally())str_match()/preg_match())str_replace()/preg_replace())What if someone doesn't like the change? No problem, just add the following to the service provider of your liking and the
Strclass will work as before:Alternatives
./src/Illuminate/Support—maybe, they would fit better into./src/Illuminate/Support/Str🤔