Repository navigation
BUGFIX: Normalize object names - #3636
Conversation
Within the object configuration and ObjectManager the usage of object names should be normalized so that fully qualified form or not will result in the same outcome, everything else can result in hard to debug errors and makes no sense either.
bd1943c to
aa432e7
Compare
|
Looks good by reading |
| * Object and class names should only register without leading backslash, but | ||
| * configuration could contain fully qualified forms. | ||
| */ | ||
| protected function normalizeObjectName(string $objectName): string |
There was a problem hiding this comment.
pitnick this method can be static and might thus be better for php optimisations? ^^
There was a problem hiding this comment.
we need the logger so forget ^^
There was a problem hiding this comment.
Im curious why do we suddenly allow backslashes? Previously its documented - even on the flow docs - that we NEVER use backslashes in class names in the front.
And dont confuse \Foo\Bar\Class::class with '\Foo\Bar\Class' - the ::class syntax same with get_class() will return non prefixed 'Foo\Bar\Class' as string.
Im not against allowing both but think we would be also in the right to reject \ correctly everywhere and that might be the way started and simpler?
But if it doesnt cost anything we can also make devs live easier and simply dont care ...
as long as we dont get in trouble where we register options for class names like:
optionsByClass:
Foo\Bar\Class:
optionA: trueAnother plugin cannot use
optionsByClass:
\Foo\Bar\Class:
optionA: falseAs wed have two keys now and get into trouble.
So from that sight it was already a good idea to simply say there is just ONE truth
|
Here is the flow documentation i read and i follow by heart:
|
|
We secretly always did allow these backslashes. In "get" it was only ever checked for interfaces. the above case exists in Flow. Changing this behaviour into a failure mode is breaking because potentially people could have above variant in their code silently doing the (depending on cofiguration) somewhat right thing unnoticed. |
I agree that we should communicate the first version as you should do that but as said, I strongly assume people have the \Foo\Bar\Class option somewhere in their code/config (just as @nezaniel ) and it worked for ages. It could break in 9.2 now if we do nothing, because 9.2 does more with the object configuration in the ObjectManager. It will fail for those cases if we throw. So IMHO the only non breaking option is this. |
|
Im still not sold. How about we at least deprecate and log this behaviour so that we can with Flow 10 fully restrict it. I think it is the Flow DNA to forbid it fully eventually because there are so many places where we expect no leading backslash.
If Neos 9.2 would already be stricter here i think we can also argue that change as anything else was never documented and the few cases users face that have not shown for exiting beta testers are minimal to adjust. I don't think this complexity is worth it if we start to make ourselves the work to allow backslashes in all cases. Also with just the one test added i think we cover really only the tip of the iceberg. So how about we keep it as is but log deprecations and continue to document not to use leading slashes. Then we can use Flow 10 to fully sort things out if we fear to do it in 9.2 or 9.3? |
|
I am not happy with the extra check this introduces but we can surely do a deprecation log here... Give me a minute. That said this is not without risk, I need to experiment a bit with it as the objectmanager is used in compiletime and I need to make sure we actually have logging available. |
mhsdesign
left a comment
There was a problem hiding this comment.
I see the need now for 9.2 and we agreed to log now and possibly remove that hack later.
We further agreed we will not document in any way that this is in-fact possible and still consider this a Flow anti pattern
Object names starting with backslashes should be logged as deprecated.
| protected function normalizeObjectName(string $objectName): string | ||
| { | ||
| if (str_starts_with($objectName, '\\')) { | ||
| $this->logger->warning('DEPRECATED: Requested object "' . $objectName . '" with leading backslash. Remove the backslash for future compatiblity.', debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 5)); |
There was a problem hiding this comment.
good one with the debug_backtrace :)
Within the object configuration and ObjectManager the usage of object names should be normalized so that fully qualified form or not will result in the same outcome, everything else can result in hard to debug errors and makes no sense either.
Specifically the following scenario is broken without this:
The problem is that without normalization the two object names are treated as differnet objects, in fact the first call will not find an object configuration and treat the object as prototype with no further object configuration.
This becomes clearer now with the ObjectManager doing the constructor auto-wiring. In that case asking for the fully-qualified format with starting backslash will simply fail because the auto-wiring does not happen, given that the ObjectManager cannot find the configuration for it (stored under the second name).
Note that this problem exists in previous versions of Flow as well, but never became visible before the ObjectManagement changes in 9.2. But for sake of backwards compatibility I think we should not change released version behavior even if it should be "safe".