While the very concept of fallbacks are undocumented (hurray...) reading the code I was able to figure out what they do. I recommend the following patch so that the next dude reading this code has an easier time.

CommentFileSizeAuthor
fallbacks_not_namespaced.patch528 byteschx

Comments

aspilicious’s picture

aspilicious commented

Chx can you explain some more why the change is needed. Or why it is an improvement. Changing this line doesn't tell me what was wrong with the previous one. Thanks!

chx’s picture

chx commented

The key is not used by Symfony and it is very confusing to have the key when you don't need it. Nonfallbacks use their key in the path you would then expect that this key means something and it doesnt. Better to remove.

robloach’s picture

robloach
he/him
commented

I'm not sure this is a good idea. This will make it so that any class that's loaded will be checked in /core/lib after the initial namespaces are checked. It's better to be verbose with the namespaces. #1658720: Use ClassLoader instead of UniversalClassLoader

robloach’s picture

robloach
he/him
commented
Status: Needs review » Postponed

Let's wait for #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) to get in too, since that's already RTBC and Angie wanted to keep the threshold down.

chx’s picture

chx commented

@RobLoach you have no idea what are you talking about I am afraid. The key in fallback is not used at all. There's nothing verbose vs terse. It's astonishing this patch haven't made it in, of course now it's obsolete but it should have been a slam dunk except noone has the slightest idea what happens because this code calls Symfony which I did read and noone else in this issue did. This is what we get for code reuse and it worries me to no end.

robloach’s picture

robloach
he/him
commented

I don't mean verbose, I mean explicit. A project should register its own namespaces. If you just register a path without the namespace, that means it'll check that path last if no other namespace check is met. I understand that both accomplish the same thing, but it's good to explicitly state where your namespaces live.

Drupal\Core lives at core/lib
Drupal\Components also lives at core/lib

Register those namespaces to their correct locations. If we don't, then "MyRandomNamespace\ChxClass" will be assumed to live at core/lib/MyRandomNamespace/ChxClass.php since "MyRandomNamespace" is not registered.

What we need are patches and benchmarks.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue summary: View changes
Status: Postponed » Needs work
Related issues: +#1424924: Use Composer for updating Symfony components (without removing Symfony code from repo)

No reason to postpone this anymore.

chx’s picture

chx commented
Status: Needs work » Closed (fixed)

Neither do we use that classloader any more.