Updated: Comment #N

Problem/Motivation

Drupal\Component\PhpStorage\PhpStorageFactory currently lives in the Component namespace, which is basically ridiculous. It's pretty Drupal specific - It requires DRUPAL_ROOT, PublicStream, Settings, and drupal_get_hash_salt(). All of which live (or soon will live) in core.

Proposed resolution

It has to move. So lets move it into Core.

Remaining tasks

Move class, untangle tests.

User interface changes

None

API changes

None. Unless you are extending the PhpStorageFactory, which is very unlikely indeed.

CommentFileSizeAuthor
#32 simpletest-to-test-cleanup-2210631-32.patch793 bytesjhedstrom
#27 2210631-27.patch17.88 KBdamiankloip
#22 2210631-22.patch17.99 KBdamiankloip
#18 2210631-18.patch18.49 KBdamiankloip
#9 2210631-9.patch18.49 KBdamiankloip
#8 interdiff-2210631-8.txt5.63 KBdamiankloip
#8 2210631-8.patch18.51 KBdamiankloip
#5 interdiff-2210631-5.txt859 bytesdamiankloip
#5 2210631-5.patch16.6 KBdamiankloip
#2 2210631.patch16.07 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Assigned: Unassigned » damiankloip

Working on this.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2210631.patch16.07 KB

I need to do a bit more work for the new PhpStorageFactory test, which I will do later on.

This is most of the patch though. I have refactored the tests that are in component to NOT use the factory class anymore. This didn't make too much sense for a unit test anyway. They should just be creating instances of the file classes directly and testing those. Which they now do. It looks much tidier tbh.

So basically:
- A new PhpStorageFactoryTest that extends DrupalUnitTestBase (IMO there is no point trying to hack and fight the dependencies on Drupal there).
- All existing file storage tests stay as PHPUnit tests.

damiankloip’s picture

damiankloip commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2210631.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2210631-5.patch16.6 KB
new interdiff-2210631-5.txt859 bytes

Should be an easy fix there.

Status: Needs review » Needs work

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

damiankloip’s picture

damiankloip commented

5: 2210631-5.patch queued for re-testing.

Hopefully that is the last time!! (#2174997: Random fail in ImageFieldDisplayTest)

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2210631-8.patch18.51 KB
new interdiff-2210631-8.txt5.63 KB

How about this? We can test it a bit better using a mock class for the php storage (in most cases). IMO this is better than trying to make a PHPUnit test containing lots of hacks for functions and constants etc..?

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2210631-9.patch18.49 KB

Rerolled.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Awesome, thank you for the reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2210631-9.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

9: 2210631-9.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

back.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2210631-9.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

9: 2210631-9.patch queued for re-testing.

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

No longer applies.

Also I marked #2033193: Move PhpStorageFactory to Core as duplicate.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2210631-18.patch18.49 KB

Pure git reroll. Thanks for closing that other issue, sorry I didn't see it before.

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2210631-18.patch no longer applies.

error: patch failed: core/tests/Drupal/Tests/Component/PhpStorage/PhpStorageTestBase.php:8
error: core/tests/Drupal/Tests/Component/PhpStorage/PhpStorageTestBase.php: patch does not apply
damiankloip’s picture

damiankloip commented

Rerolling...

damiankloip’s picture

damiankloip commented
Issue tags: -Needs reroll
StatusFileSize
new 2210631-22.patch17.99 KB
damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
damiankloip’s picture

damiankloip commented

22: 2210631-22.patch queued for re-testing.

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

Back

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2210631-22.patch no longer applies.

error: patch failed: core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php:62
error: core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php: patch does not apply
damiankloip’s picture

damiankloip commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
StatusFileSize
new 2210631-27.patch17.88 KB

And once more. Just context resolution.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 2aefd89 and pushed to 8.x. Thanks!

  • Commit 2aefd89 on 8.x by alexpott: 
    Issue #2210631 by damiankloip: Move PhpStorageFactory to the Core...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2210631-27.patch no longer applies.

error: patch failed: core/includes/common.inc:15
error: core/includes/common.inc: patch does not apply
error: patch failed: core/includes/utility.inc:6
error: core/includes/utility.inc: patch does not apply
error: patch failed: core/lib/Drupal/Core/DrupalKernel.php:7
error: core/lib/Drupal/Core/DrupalKernel.php: patch does not apply
error: core/lib/Drupal/Component/PhpStorage/PhpStorageFactory.php: No such file or directory
error: patch failed: core/lib/Drupal/Core/Template/TwigEnvironment.php:7
error: core/lib/Drupal/Core/Template/TwigEnvironment.php: patch does not apply
error: patch failed: core/modules/language/language.module:5
error: core/modules/language/language.module: patch does not apply
error: patch failed: core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php:7
error: core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php: patch does not apply
error: patch failed: core/modules/language/lib/Drupal/language/EventSubscriber/ConfigSubscriber.php:7
error: core/modules/language/lib/Drupal/language/EventSubscriber/ConfigSubscriber.php: patch does not apply
error: core/modules/system/lib/Drupal/system/PhpStorage/MockPhpStorage.php: already exists in working directory
error: core/modules/system/lib/Drupal/system/Tests/PhpStorage/PhpStorageFactoryTest.php: already exists in working directory
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.php:8
error: core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php:7
error: core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php:7
error: core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Component/PhpStorage/PhpStorageTestBase.php:8
error: core/tests/Drupal/Tests/Component/PhpStorage/PhpStorageTestBase.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php:7
error: core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php: patch does not apply
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Fixed
Issue tags: -Needs reroll

Huh... this is fixed

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Fixed » Needs review
StatusFileSize
new simpletest-to-test-cleanup-2210631-32.patch793 bytes

This patch missed one occurrence of php/simpletest. Presumably this still passes on the bot since that directory exists. Locally it fails since that directory is never created (unless running simpletest).

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

Phpunit was failing on HEAD on my local machine without this patch. Patch applies, and all tests now pass.

damiankloip’s picture

damiankloip commented

Good catch! +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 2cc71a6 and pushed to 8.x. Thanks!

  • Commit 2cc71a6 on 8.x by alexpott: 
    Issue #2210631 followup by jhedstrom: Move PhpStorageFactory to the Core...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.