Make WordPress Core

Opened 8 months ago

Closed 7 months ago

Last modified 7 months ago

#59406 closed defect (bug) (fixed)

Undefined and unused variable in WP_Test_Stream::mkdir

Reported by: davidbinda's profile david.binda Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.5 Priority: normal
Severity: normal Version: 5.6
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The $file variable is undefined in https://core.trac.wordpress.org/browser/trunk/tests/phpunit/includes/class-wp-test-stream.php?rev=55017#L206

Further, the $plainfile is not being used: https://core.trac.wordpress.org/browser/trunk/tests/phpunit/includes/class-wp-test-stream.php?rev=55017#L204

Since I haven't been able to find a test which would be using the mkdir method of the stream wrapper, I'm unable to tell what would be the best approach to fix the method, since replacing the $file by $this->file nor by $plaifile does not seem to prevent repeated directory creations.

I'm attaching a patch which, IMHO, makes the method to work, but I'm not sure if it's the best approach.

Attachments (1)

59406.diff (638 bytes) - added by david.binda 8 months ago.

Download all attachments as: .zip

Change History (12)

@david.binda
8 months ago

#1 @SergeyBiryukov
8 months ago

  • Component changed from General to Build/Test Tools
  • Milestone changed from Awaiting Review to 6.4

This ticket was mentioned in PR #5300 on WordPress/wordpress-develop by @sadizaman.


8 months ago
#2

  • Keywords has-patch has-unit-tests added

Trac ticket: 59406

---
This ticket already has a patch. I have just updated the current patch. Looking for suggestion.

#3 in reply to: ↑ description @SergeyBiryukov
7 months ago

  • Version set to 5.6

Thanks for the ticket! Introduced in [49230] / #42663.

Replying to david.binda:

Since I haven't been able to find a test which would be using the mkdir method of the stream wrapper, I'm unable to tell what would be the best approach to fix the method, since replacing the $file by $this->file nor by $plaifile does not seem to prevent repeated directory creations.

At a glance, it seems like the intention was to prevent directory creation if there is already a file with the same name. However, repeated directory creations should probably be prevented too, that would be consistent with the PHP mkdir() implementation.

So I think the patch may need some adjustment to do both.

#4 @SergeyBiryukov
7 months ago

  • Keywords changes-requested added

This ticket was mentioned in PR #5557 on WordPress/wordpress-develop by @rajinsharwar.


7 months ago
#5

Preventing the creation of duplicate directories of the mkdir() function

Trac ticket: https://core.trac.wordpress.org/ticket/59406

#6 @rajinsharwar
7 months ago

  • Keywords changes-requested removed

Added the check for duplicate directories as well @SergeyBiryukov in the current patch.

#7 @SergeyBiryukov
7 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#8 @SergeyBiryukov
7 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 56998:

Tests: Correct the WP_Test_Stream::mkdir() method.

The method attempted to check if there is already a file with the same name, however the conditional used an undefined variable.

This commit prevents directory creation if a file or directory with the same name already exists, bringing consistency with the PHP mkdir() implementation.

Includes adding missing documentation for the method.

Reference: PHP Manual: streamWrapper::mkdir().

Follow-up to [49230].

Props david.binda, sadizaman, rajinsharwar, SergeyBiryukov.
Fixes #59406.

#9 @SergeyBiryukov
7 months ago

  • Milestone changed from 6.4 to 6.5

Since it's not a regression in 6.4, I think this ticket can be moved to 6.5 and does not need a backport.

@SergeyBiryukov commented on PR #5300:


7 months ago
#10

Thanks for the PR! Merged in r56998.

@SergeyBiryukov commented on PR #5557:


7 months ago
#11

Thanks for the PR! Merged in r56998.

Note: See TracTickets for help on using tickets.