Problem/Motivation

  1. The class is set to SplString which is a PECL only class. It simply doesn't exist. Try php --rc SplString.
  2. 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.
  3. 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Assigned: Unassigned » dawehner

Well, we just did it liked that because it worked, see https://www.drupal.org/node/2328111#comment-9164137 why we need an actual factory.

chx’s picture

Could 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.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Active » Needs review
FileSize
3.02 KB

Let's see whether this works.

Status: Needs review » Needs work

The last submitted patch, 3: 2536012-3.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
525 bytes

Let's try that.

Status: Needs review » Needs work

The last submitted patch, 5: 2536012-5.patch, failed testing.

znerol’s picture

+++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -97,7 +97,7 @@ class ExtensionDiscovery {
-    $this->root = $root;
+    $this->root = (string) $root;

I guess we should do that in all cases, this will also avoid repeated unnecessary calls to __toString().

chx’s picture

Status: Needs work » Needs review
znerol’s picture

Or directly inject the factory and call getAppRoot() in constructors.

dawehner’s picture

Or directly inject the factory and call getAppRoot() in constructors.

For 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?

znerol’s picture

The 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.

dawehner’s picture

The 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.

Yeah ... it is also even for some docker environments that the CLI path is different to the one apache sees.

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.

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.

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.

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.

znerol’s picture

The 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:

  1. No type hinting, but cope with the fact that the parameter can be a string or an object with __toString() method.
  2. Type hint 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 a getAppRoot() method which returns a scalar is not really a factory.

chx’s picture

> 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?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

neclimdul’s picture

This seems to still be valid?

neclimdul’s picture

"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."

Mile23’s picture

Status: Needs review » Needs work
Related issues: +#2631362: Inject DRUPAL_ROOT into DrupalKernel

What 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

Mile23’s picture

Version: 8.1.x-dev » 8.2.x-dev
neclimdul’s picture

1/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.

neclimdul’s picture

Would like to decide on how core address the options from #13 before implementing using \SplFileInfo or re-rolling this.

  1. No type hinting, but cope with the fact that the parameter can be a string or an object with __toString() method.
  2. Type hint StringObject and loose the option to pass in a string, e.g. in unit tests.

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:

    // This sets a value on the array as a StringObject object.
    $searchdirs[static::ORIGIN_SITE] = \Drupal::service('site.path');
    // ...
    foreach ($searchdirs as $dir) {
      // $dir is an object and an invalid index.
      if (!isset(static::$files[$dir][$include_tests])) { 
         // ...
      }
    }
    // ...
    // Again, the object would become a key so array_flip bails.
    $origin_weights = array_flip($searchdirs); 
Mile23’s picture

I'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 have app.root be a service to convert that to a string. And immediately mark it as deprecated.

dawehner’s picture

If needed, we could transition using a different parameter like app.root_directory, and have app.root be a service to convert that to a string. And immediately mark it as deprecated.

Well, 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

neclimdul’s picture

Hey 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.

neclimdul’s picture

Status: Needs work » Needs review

lets see what testbot thinks.

Status: Needs review » Needs work

The last submitted patch, 27: 2536012-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

neclimdul’s picture

Documented in the IS but never addressed. Here's an issue for PublicStream::basePath() #2934725: PublicStream::basePath argument is invalid

Mile23’s picture

Title: The app.root service is invalid » The app.root and site.path services are invalid

It's the same problem in site.path.

+1 on adding a StringObject like in #27.

neclimdul’s picture

Its 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.

dawehner’s picture

I 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.

neclimdul’s picture

This 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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

donaldinou’s picture

Why not just using a polyfill for \SplString instead of develop custom String Object?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Version: 8.9.x-dev » 9.0.x-dev

This looks 9.x material but there was duplicate with fresher patch

andypost’s picture