Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- The class is set to
SplString
which is a PECL only class. It simply doesn't exist. Tryphp --rc SplString
. - Consequently,
PublicStream::basePath
can not be called with an argument because it's typehinted with a nonexisting class. It is not called indeed, only the doxygen suggests doing that. - The factory is set to app.root.factory:get which returns a string violating the Container::get contract where services return objects.
Proposed resolution
Actually find/write a class with __toString ...
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#27 | 2536012-27.patch | 3.75 KB | neclimdul |
Comments
Comment #1
dawehnerWell, we just did it liked that because it worked, see https://www.drupal.org/node/2328111#comment-9164137 why we need an actual factory.
Comment #2
chx CreditAttribution: chx commentedCould it be a parameter instead of a service? It feels like a string should be a parameter. If it can't be , we need a class with __toString.
Comment #3
dawehnerLet's see whether this works.
Comment #5
dawehnerLet's try that.
Comment #7
znerol CreditAttribution: znerol commentedI guess we should do that in all cases, this will also avoid repeated unnecessary calls to
__toString()
.Comment #8
chx CreditAttribution: chx commentedComment #9
znerol CreditAttribution: znerol commentedOr directly inject the factory and call
getAppRoot()
in constructors.Comment #10
dawehnerFor some special cases maybe that would good, but I disagree with that idea. This would couple things to the factory even ideally you should be able to write classes without that implementation detail, don't you think so?
Comment #11
znerol CreditAttribution: znerol commentedThe problem is that we treat the app root like a constant for historical reasons. However, it is not exactly a constant but more like an environment variable. It does not change during a request, not even across container rebuilds. But it might be different across deployments. In theory it could be different on every webhead in a loadbalancing setup. That is an additional reason why it wouldn't be save to store it as a container parameter.
It looks like we need to touch many constructors and introduce an explicit typecast if we stick with the
__toString()
approach. Contrib and custom code will have to adapt that pattern otherwise people will run into weird bugs.Instead we could introduce a service with a method which returns the app root as a string with the additional benefit of getting properly type hinted constructors.
Comment #12
dawehnerYeah ... it is also even for some docker environments that the CLI path is different to the one apache sees.
Do you mind explaining why? If someone runs into that weird bug, they can step print out the variable, see its an object with
__toString()
and can typehint on their own.I don't see why core should be adapted. It is kinda an implementation detail that an object is used here, so people should NOT rely on it.
Well this is the factory. According to https://wiki.php.net/rfc/scalar_type_hints_v5 it is fine to use an object for 'string' scalar typehinting.
Comment #13
znerol CreditAttribution: znerol commentedThe PHP version required by Drupal 8 will not have scalar type hints. Hence there are two options for code accepting the app.root regarding type hinting:
__toString()
method.StringObject
and loose the option to pass in a string, e.g. in unit tests.Also in that PHP version, scalar strings behave quite different than objects with the
__toString()
magic method as indicated by the test failures. Grepping for$this->root\W
indicates that the value is used in obviously scalar contexts. It seems natural to ensure that the root property of any class is a scalar value and not an object.We need to touch all classes where the app root service is injected. This is because the container requires objects but the
$root
property is expected to be a scalar. In my opinion it is preferable to be explicit about this situation and inject an app-root-provider into services instead of a non-type-hintable magic string object. A service with agetAppRoot()
method which returns a scalar is not really a factory.Comment #14
chx CreditAttribution: chx commented> Also in that PHP version, scalar strings behave quite different than objects with the __toString() magic method as indicated by the test failures.
Well, as we learned in the autoescape patch , there are different behaviors but why are we getting those? Like, you can't index with a __toString object like you can with a string because it's not auto-cast but one would believe we always concat something to the app root thus forcing it to string. So... what happens that breaks this object?
Comment #16
neclimdulThis seems to still be valid?
Comment #17
neclimdul"Ugh, who added that broken code to PublicStream::basePath(). Clearly not tested, anyone that reviewed that issue should be shot."
git blame
"On second thought, its an honest mistake anyone could have made. Even the most thorough developer might have overlooked it. Top notch developers on that issue."
Comment #18
Mile23What we should have:
1) The front controller should figure out DRUPAL_ROOT.
2) The FC should then inject the root to the kernel on creation.
3) The kernel should set it as a container parameter. It really should just be a string, or maybe better yet a SplFileInfo-type object. It should be something that can be faked out by vfsStream.
4) All dependent code (that is, everything else) should then use dependency injection to gather the root from the container. Core will need a bunch of work in this regard.. My IDE says there are 120 occurrences of DRUPAL_ROOT in 8.2.x.
The reason we have a factory is because we failed to do all these things and we needed a workaround.
Work on items 1 and 2 is here: #2631362: Inject DRUPAL_ROOT into DrupalKernel
Comment #19
Mile23Comment #20
neclimdul1/2) While useful, I'm sure you're aware that's only a smidge of a change from the current code in the kernel we're already using. It also seems only tangential to the fact that we're breaking the Container contract.
3) in #13 it was argued this should not be a parameter. Lets address that.
\SplFileInfo is an interesting option. It is descriptive, more useful an object, and can be treated as a string the same way "StringObject" from previous patches can.
4) Right, but this is addressing code already using injection though. As long as we are storing scalars(strings) as a service object in the container things could break.
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Dep...
It only currently works because PHP is loosely typed and the current Symfony implementation doesn't do anything object specific with the values. While I don't expect it ever will, Symfony could do something like use spl_object_hash() on the object and break our code at any point. This issue was actually triggered by the fact that #2531564: Fix leaky and brittle container serialization solution has to add a is_object() check around spl_object_hash() to avoid breaking.
Comment #21
neclimdulWould like to decide on how core address the options from #13 before implementing using \SplFileInfo or re-rolling this.
For example, does Drupal::root() return a string or object? Does ExtensionDiscovery expect root and site path to be mixed and work around that(currently they come from all sorts of places not just the container and would be mixed without changes outside the class). Does PublicStream::basePath() start expecting a string or object?
re:#14
The failures I saw just skimming in ExtensionDiscovery where because of using a object as a key to an array. Like:
Comment #22
Mile23I'm pretty sure the point of this issue is to define the service parameter, and change core to consume it.
If needed, we could transition using a different parameter like
app.root_directory
, and haveapp.root
be a service to convert that to a string. And immediately mark it as deprecated.Comment #23
dawehnerWell, this is what we do. The factory calls out to the DrupalKernel, which has the string version stored. The only reason why we have \SplString there is to convince the container about it.
Comment #27
neclimdulHey there old issue...
For giggles here's the last implementation rerolled. Its functional we just have the BC issue of StringObject not being a string and that messing up things that where using root as an array index because that doesn't type coerce and fatals. Also its change in signatures technically being a BC break.
Note: update has a similar service doing the same thing now we should address here or in a follow up if we decide to move forward.
Comment #28
neclimdullets see what testbot thinks.
Comment #30
neclimdulDocumented in the IS but never addressed. Here's an issue for PublicStream::basePath() #2934725: PublicStream::basePath argument is invalid
Comment #31
Mile23It's the same problem in site.path.
+1 on adding a StringObject like in #27.
Comment #32
neclimdulIts a pretty straight forward solution but Testbot clearly didn't like the coercion. Lots of offset errors. We're going to have to go with the more gentle deprecation I guess though I'm not sure how that's going to happen and have everything be a valid service.
Comment #33
dawehnerI totally understand the bug. Could we actually fix it upstream and allow also primitive types for services? I guess we would then be done as well.
As we see in the test failures this change wouldn't be without a risk.
Comment #34
neclimdulThis issue has been around a long time and its sad that I don't see this documented but I'm pretty sure I remember that being discussed and Symfony's response was "a primitive isn't a service" which is technically correct. I'm opening to opening the dialog again. Honestly if it wasn't for BC there would be a lot of options open to us outside of the string object like just using a parameter.
Comment #38
donaldinou CreditAttribution: donaldinou commentedWhy not just using a polyfill for \SplString instead of develop custom String Object?
Comment #40
andypostThis looks 9.x material but there was duplicate with fresher patch
Comment #41
andypostNow it duplicates #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters