fix: handle short classes in Autoloader #41253
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
phil-davis
force-pushed
the
fix-autoloader-findClass
branch
from
a295871
to
38fa15a
Compare
11 tasks
|
Do we know which classes are complained about? If we expect classes with 3 components and somehow we get classes with just 2, either sending any class with 2 components is a bug (and we mustn't use those) or we should handle those classes better. |
No, but I suppose that there are 6 of them, the number of notices reported. I will try locally to log the ones with the problem and report back... |
|
@jvillafanez there are 4 different values for and I suppose if more apps are enabled, then there could be more similar examples passed in. |
|
Is it possible to get a stacktrace to know the whole list of calls? I mean, we're trying to load the |
|
I think that The call stack has this sort of stuff (sorry,it's long): It starts getting the idea that maybe And that ends up diving way down into the Autoloader. I suppose that there is a problem somewhere in all the Now to think what to do next. It's not going to be simple to understand the whole path that phpstan goes through to find all the classes and load them, and then to find the point where something is a little bit wrong. |
|
I have suspicions on https://github.com/search?q=repo%3Aowncloud%2Fcore%20loadAdditionalScripts&type=code and https://github.com/search?q=repo%3Aowncloud%2Fcore%20remote_shareReceived&type=code Those are event names, but maybe phpstan is interpreting them as valid / possible methods that could be called. I don't know if it's worthy to change the names of those events to figure out whether that's the issue, because in the end we can't change the names (we could break apps) Assuming that's basically the problem, we'll probably have to workaround with something similar to this PR. In this case, returning an empty array if there aren't enough components seems a good choice We should include a comment to explain the situation because I don't think it's a good solution, but I don't see other viable alternatives |
phil-davis
force-pushed
the
fix-autoloader-findClass
branch
from
38fa15a
to
929c0a0
Compare
Agree. I added code to return early when the problem happens, and a comment (that got quite long!). Note: we have to adjust the @jvillafanez please review again. |
jvillafanez
approved these changes
May 16, 2024
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
See the issue for details of the problem.
If
explodedoes not return enough parts, pad the returned array with empty strings so that thelist()will have a big enough array to fill the variables inlist().Related Issue
How Has This Been Tested?
CI
Types of changes
Checklist: