Spin-off from #1320648: Meta: start converting existing core classes to PSR-0 [policy, no patch].

CommentFileSizeAuthor
#1 stream_wrappers-psr0.patch43.97 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Assigned: amateescu » Unassigned
Status: Active » Needs review
FileSize
stream_wrappers-psr0.patch43.97 KB

Noteworthy things about this patch:

  • in lack of a better place for now, constant definitions from stream_wrappers.inc have been moved to file.inc
  • interface renames:
    StreamWrapperInterface -> Drupal\Core\StreamWrapper\PhpStreamWrapperInterface
DrupalStreamWrapperInterface -> Drupal\Core\StreamWrapper\StreamWrapperInterface
  • class renames (added 'Stream' to all of them because we can't use 'Public' or 'Private'):
    DrupalLocalStreamWrapper -> Drupal\Core\StreamWrapper\LocalStream
DrupalPublicStreamWrapper -> Drupal\Core\StreamWrapper\PublicStream
DrupalPrivateStreamWrapper -> Drupal\Core\StreamWrapper\PrivateStream
DrupalTemporaryStreamWrapper -> Drupal\Core\StreamWrapper\TemporaryStream
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

This mostly makes sense to me. I get dropping the 'Drupal' part from the class names, but it seemed odd to me that the classes now just have 'Stream' as a suffix rather than 'StreamWrapper' or no suffix at all.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

After a little more review I'm totally fine with the renaming proposal in #1 based on the PHP example 'VariableStream' from http://us.php.net/manual/en/stream.streamwrapper.example-1.php. Makes sense for us to follow along. I don't quite feel comfortable marking this as RTBC so I'd like to leave that to another reviewer a little better versed in the PSR-0 conversions.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
@@ -2,247 +2,34 @@
-   * @var Resource
+   * @var resource

Resource isn't a class. It's a PHP language type, like int or string. So it should probably stay lowercase.

I would also recommend putting the free-floating constants in the interface. They can lazy-load that way, and it's a model that Symfony is already using in places (HttpKernelInterface, for instance) so it would be consistent.

I'm fine with the class renames.

19 days to next Drupal core point release.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review

Resource isn't a class. It's a PHP language type, like int or string. So it should probably stay lowercase.

Wut? The patch is changing it to lowercase. You meant leaving it as title case?

I would also recommend putting the free-floating constants in the interface. They can lazy-load that way, and it's a model that Symfony is already using in places (HttpKernelInterface, for instance) so it would be consistent.

I don't think that's possible atm, because file.inc uses them before initializing any stream wrapper class, for example: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/inclu...

Maybe we should leave this for a file.inc clean-up issue?

I'm fine with the class renames.

Yay :)

Crell’s picture

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

Er. I can't read. Yes, you're lowercasing them, which is correct.

For the constants on the interface, as long as the interface is loadable by the class loader before file.inc is included, it will work. That's the whole point of the class loader. :-)

That said, some of them (the ones that are define()s now) couldn't do that anyway, since class constants like global const are compile-time specified, not runtime. So... eh, yeah, I guess we could leave them. No strong preference either way.

Yeah, let's just go ahead and do it then. :-)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Great, this all looks good. Committed to 8.x. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Convert stream_wrappers.inc to PSR-0 » [HEAD BROKEN] Convert stream_wrappers.inc to PSR-0
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

The new files were not added.

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Classes should be named LocalStreamWrapper not LocalStream IMHO.

EDIT: "Local" would mean something, but would be confusing when used without alias in external code, LocalStreamWrapper is explicit and a good name to use, while "LocalStream" is an intermediate name that doesn't mean anything: "Stream" is a too common word and might be used in many other contextes, it has nothing to do with "StreamWrapper", I think we should go for fully explicit name.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

@pounard, see #3.

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Indeed, it's still sounds weird. If the interface is named "StreamWrapper" then classes should be named "SomethingStreamWrapper", if we go for "SomethingStream" the interface should be renamed too.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: [HEAD BROKEN] Convert stream_wrappers.inc to PSR-0 » Convert stream_wrappers.inc to PSR-0
Status: Reviewed & tested by the community » Fixed

Ok. After about 30 minutes of back and forth with sun and some other folks in IRC, I think I managed to fix this. ;)

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