#1444620: Remove file signing from configuration system removed the file signing, but didn't remove the "Verified" from Drupal\Config class names.

Also, the class names could use some heavy clean-up.

CommentFileSizeAuthor
#9 config.classes.9.patch13.75 KBsun
#6 config.classes.6.patch13.72 KBsun
#5 config.classes.5.am_.patch18.69 KBsun
#5 config.classes.5.patch13.78 KBsun
#2 config.classes.2.patch18.69 KBsun
#1 config.classes.1.patch8.99 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
config.classes.1.patch8.99 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
config.classes.2.patch18.69 KB

Adjusted docs and renamed DrupalVerifiedStorageSQL into DatabaseStorage.

Status: Needs review » Needs work

The last submitted patch, config.classes.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

FWIW, with this patch, core\lib\Drupal\Core\Config> dir /B yields:

AbstractStorage.php
ConfigException.php
DatabaseStorage.php
DrupalConfig.php
FileStorage.php
FileStorageException.php
FileStorageReadException.php
StorageInterface.php
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
config.classes.5.patch13.78 KB
config.classes.5.am_.patch18.69 KB

Odd. #2 should apply cleanly. Trying again.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
config.classes.6.patch13.72 KB

Briefly discussed the "Abstract" prefix in IRC. Removed it, as it's not strictly necessary and lines up Storage.php with StorageInterface.php:

ConfigException.php
DatabaseStorage.php
DrupalConfig.php
FileStorage.php
FileStorageException.php
FileStorageReadException.php
Storage.php
StorageInterface.php
yched’s picture

yched CreditAttribution: yched commented

Denoting the abstract nature of the class within the class name is precious IMO.
Shouldn't be a blocker for this patch, but I wish we settled some convention regarding those "abstract" classes - opened #1567920: Naming standard for abstract/base classes about that.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs review » Needs work

I don't have a strong feeling on this but based on the results of #1567920: Naming standard for abstract/base classes it appears that a standard has been essentially decided, so I'm putting this to CNW since it will undoubtedly need a reroll.

Otherwise I'm liking this a lot. Thanks sun.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
config.classes.9.patch13.75 KB

Renamed Storage to StorageBase.

FWIW, this change is also available as a pull-able branch in cmi/config-classes-1567812-sun

(At some point, I'd like to have some clarity on what patches will be merged and which not, 'cos creating and maintaining feature branches involves a certain overhead.)

Status: Needs review » Needs work
Issue tags: -API clean-up, -Configuration system

The last submitted patch, config.classes.9.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
Issue tags: +API clean-up, +Configuration system

#9: config.classes.9.patch queued for re-testing.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

I haven't been maintaining feature branches for anything of late, although I was thinking about it for the rearchitecture issue since it is a more colossal change. I agree it is all pretty loose at this point and it would be nice to have some standards.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Alright, "Drupal installation failed" was a testbot hiccup. Ready to fly?

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs review » Reviewed & tested by the community

Ayup

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#9: config.classes.9.patch queued for re-testing.

catch’s picture

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

We're over thresholds but internal stuff in cmi can't possibly conflict with any critical issues so I've gone ahead and committed this one.

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