[5.5] Mix refactoring #19895
[5.5] Mix refactoring #19895
Conversation
* laravel/master: (519 commits) [5.5] Remove useless variables. (laravel#19866) Ensure a 304 response is properly returned. (laravel#19867) Apply fixes from StyleCI (laravel#19876) Stay consistant with expected exceptions and test messages. (laravel#19873) formatting formatting Improve ThrottleRequests. Allow passing of data to the view Apply fixes from StyleCI (laravel#19852) Testing existing feature (laravel#19847) verison Call parent::handle command (laravel#19839) Update ViewController.php Apply fixes from StyleCI (laravel#19836) Update CallbackEvent.php (laravel#19834) Add Route::view() helper Use proper assertions. (laravel#19831) Make illuminate/container require psr-11 (laravel#19829) Fix some docblocks and missing properties. (laravel#19825) formatting ... # Conflicts: # src/Illuminate/Foundation/helpers.php
|
The manifest cache issue should be fixed in a separate PR to 5.4. It shouldn't be a part of this other huge PR. |
|
@taylorotwell yeah, of course we'll have to fix it also in 5.4 if you don't want to merge this one in it. But for 5.5, it doesn't change anything, because in any way, this PR refactors the entire function. The previous bug, in the previous function doesn't exist anymore. |
| { | ||
| $manifest = $this->getManifest($manifestDirectory); | ||
|
|
||
| if (array_key_exists($path, $manifest)) { |
There was a problem hiding this comment.
Can we change this to use isset instead? It would improve the performance a little bit...
It seems that null shouldn't be an expected value here anyway so the change in behavior should'nt matter...
Same for the one below.
There was a problem hiding this comment.
Yeah sure, no problem for me, I didn't know that.
I'll wait a bit before pushing it, just in case of other modifications to do.
|
This won't go into 5.4. |
|
What problem is this trying to solve? |
|
@devcircus see #19639 for the initial explanation. |
|
Let's maybe try to make smaller, focused, incremental improvements to Mix instead of re-writing it into this class. |
|
Guy really ? I'm proposing a fully tested and complete proper refactoring after a discussion in internals. I'm adding some demanded features, even by you apparently:
There is no breaking change here., and an user who know nothing about the new features will not see the difference. I've considered each comment and changed my code with all your feedback, and now after three weeks of waiting, you're closing it without any real explanation? What's the problem with that now? |
Third try...
I've fixed the #19764 which made things worst than before as nothing isn't cached anymore... 😓
(see #19764 (comment). Imho we really shouldn't merge untested code!)
And I tried to do what you asked here : #19666 (comment). Imo this was clearer and prettier before, but I can understand the need if it's for performances.
@taylorotwell I've merged 5.5 commits in it as you asked, but I really think it should go on 5.4 too, because it fix an issue, and add a feature that lot of people is waiting for.
Let me know if you see something else.