Problem/Motivation

SplString is not a valid class within the requirements of a Drupal install as it comes from pecl. This means that most users can't run code that passes an argument to this function and core and contrib can't use it.

Proposed resolution

I think there are 2 options:
1) The code has been broken and unreported for 2.5 years so we could just remove it.
2) Remove the type hint and make the code functional.

Remaining tasks

Patches. Review.

User interface changes

n/a

API changes

Technically an API change because the signature is changing but the current signature is only functional in the first place because the argument is optional and never used so the type is never looked up.

Data model changes

n/a

CommentFileSizeAuthor
#11 2934725-11.interdiff.txt2.55 KBneclimdul
#11 2934725-option2-11.patch3.31 KBneclimdul
#7 2934725-option2-7.patch2.63 KBneclimdul
#2 2934725-option2-1.patch1.13 KBneclimdul
#2 2934725-option1-1.patch2.1 KBneclimdul

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Needs review
StatusFileSize
new 2934725-option1-1.patch2.1 KB
new 2934725-option2-1.patch1.13 KB

If we do go with option 2 we should add tests because clearly there is no coverage currently. Option 1 doesn't need any additional testing, we're just removing dead code.

neclimdul’s picture

neclimdul
He/Him
commented
Related issues: +#2384675: Deprecate conf_path()

Issue where code was added for reference.

andypost’s picture

andypost
he/him
Primary language Russian
commented

+1 to option2

neclimdul’s picture

neclimdul
He/Him
commented

I'm partial to 1 as the code isn't being used indicating a lack of interest and code that doesn't exist doesn't have bugs but which ever.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2536012: The app.root and site.path services are invalid

PublicStream::basePath() says this:

   * The site path is injectable from the site.path service:
   * @code
   * $base_path = PublicStream::basePath(\Drupal::service('site.path'));
   * @endcode

That looks reasonable, because these are a bunch of interdependent services that could be in use outside core.

So it's the same problem as #2536012: The app.root and site.path services are invalid which is that \SplString shouldn't be a thing we use. I couldn't find an issue for site.path to go along with app.root, so I'm adding site.path to that issue. Whatever's decided over there will inform what to do here since this code consumes the service.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new 2934725-option2-7.patch2.63 KB

"That looks reasonable"
I think you wrote it so that makes sense. ;) The point I was trying to make is ATM that's not a real use case.

2 votes though so here's option 2 with tests. Bonus fact, we had not unit tests for this so more tests!

Status: Needs review » Needs work

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

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

"That looks reasonable"
I think you wrote it so that makes sense. ;)

Yah I guess it's sort of retroactive self-RTBC. :-)

The fail says:

1) Drupal\Tests\Core\StreamWrapper\PublicStreamTest::testBasePathSettings
Error: Call to undefined function Drupal\Core\drupal_valid_test_ua()

Switch to kernel test.

+++ b/core/tests/Drupal/Tests/Core/StreamWrapper/PublicStreamTest.php
@@ -0,0 +1,43 @@
+ * Tests the TranslatableMarkup class.

Ewps.

neclimdul’s picture

neclimdul
He/Him
commented

bah, took a quick peak and there's a test adding drupal_valid_test_ua() so phpunit runner runs clean but testbot doesn't. Will take care of it later today.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new 2934725-option2-11.patch3.31 KB
new 2934725-11.interdiff.txt2.55 KB

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.

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.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

neclimdul’s picture

neclimdul
He/Him
commented

Don't think this really matters anymore or would apply after #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Closed (outdated)

also #2234479: Deprecate hook_test_* hooks in simpletest made some of these changes. Just going to close.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3239840: Throw exception in \Drupal\Core\StreamWrapper\PrivateStream::getDirectoryPath() when basepath() is null

Follow-up to clean-up