Problem/Motivation

While working on #1308152: Add stream wrappers to access extension files I found this: Instead of calling the parent method LocalStream::stream_open(), \Drupal\Core\StreamWrapper\LocalReadOnlyStream::stream_open() is duplicating the code.

Proposed resolution

Cleanup.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 remove_code_duplication-2751351-2.patch1.69 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Version: 8.2.x-dev » 8.1.x-dev
Status: Active » Needs work
FileSize
remove_code_duplication-2751351-2.patch1.69 KB
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue tags: +code cleanup
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Parent issue: » #1308152: Add stream wrappers to access extension files
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue tags: +DevDaysMilan
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Good, valid, clean up!

I think my only nit pick would be a couple of inline comments to explain what this is doing different to the parent, because we are now losing the * Any write modes will be rejected, as this is a read-only stream wrapper. comment.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

@timmillwood, thank you for review. I was tempted to add a comment but then I saw this line that seems self-explanatory for me:

trigger_error('stream_open() write modes not supported for read-only stream wrappers', E_USER_WARNING);
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood for Pfizer, Inc. commented

@claudiu.cristea - Yeh, I just wonder if it'd be move evident if bubbled up in a comment rather than in a function call. Although not overly precious about this, which is why I RTBC'd it rather moving it to needs work.

I guess whichever core committer looks at this can decide to add a comment or not on commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed f84b2a1 and pushed to 8.2.x. Thanks!

As a task this is only eligible for 8.2.x - I agree with @claudiu.cristea - I think the code and trigger_error is self documenting.

  • alexpott committed f84b2a1 on 8.2.x 
    Issue #2751351 by claudiu.cristea: Remove code duplication from...
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue tags: -DevDaysMilan

Status: Fixed » Closed (fixed)

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