WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 6 days ago

Last modified 5 days ago

#44533 closed defect (bug) (wontfix)

wp_is_stream wrappers need preg delimiter when quoting

Reported by: JPry Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

With change r42432, the stream wrappers are now quoted with preg_quote() before being passed into preg_match(). However, the call to preg_match() is using ! as a delimiter. This means that the calls to preg_quote() should also have that delimiter included as the second parameter.

Attachments (2)

44533.patch (653 bytes) - added by JPry 2 weeks ago.
44533.2.patch (1.8 KB) - added by JPry 2 weeks ago.
Same as first patch, but with unit tests added

Download all attachments as: .zip

Change History (12)

@JPry
2 weeks ago

#1 @JPry
2 weeks ago

  • Keywords has-patch added

@JPry
2 weeks ago

Same as first patch, but with unit tests added

#2 @JPry
2 weeks ago

  • Keywords has-unit-tests added

I decided to go back and add some unit tests for wp_is_stream(). The second patch includes those tests.

#3 @SergeyBiryukov
2 weeks ago

  • Milestone changed from Awaiting Review to 4.9.8

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


10 days ago

#5 @pento
6 days ago

  • Milestone 4.9.8 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Thank you for the report and patch, @JPry!

I was thinking about this, but we don't really need to make the change, for two reasons:

  • ! is a special character that preg_quote() already escapes.
  • ! isn't a character that PHP allows in stream names, so if it shows up in $wrappers, things have gone horribly wrong.

#6 @JPry
5 days ago

Thanks for the explanation @pento

What about the unit tests? Would these still be helpful for wp_is_stream() in general? They weren't particular to the use of preg_quote(), and I don't think there's currently any coverage for wp_is_stream() in core.

#7 @SergeyBiryukov
5 days ago

In 43501:

Filesystem API: Add basic tests for wp_is_stream().

Props JPry.
See #44533.

#8 @pento
5 days ago

  • Keywords needs-unit-tests needs-patch added; has-patch has-unit-tests removed
  • Milestone set to 5.0
  • Version trunk deleted

The tests are failing on PHP 5.2:

1) Tests_Functions::test_wp_is_stream with data set #0 ('https://example.com', true)
Failed asserting that false matches expected true.
/home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/functions.php:1464

https://travis-ci.org/WordPress/wordpress-develop/jobs/405160164

It appears that Travis' PHP 5.2 build doesn't have openssl built in, so https is not a stream that will actually work. 🙂

I would suggest testing for extension_loaded( 'openssl' ), and use that to decide whether a http or https URL should be used.

This ticket was mentioned in Slack in #core by pento. View the logs.


5 days ago

#10 @SergeyBiryukov
5 days ago

In 43503:

Filesystem API: Skip https:// test for wp_is_stream() if openssl extension is not loaded.

See #44533.

Note: See TracTickets for help on using tickets.