Skip to content

Refactor plain wrapper to handle mkdir itself #15520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 21, 2024

This effectively inlines the behaviour of php_mkdir_ex() which is a deprecated API from at least 17 years ago, and also fixes some of the return values

This effectively inlines the behaviour of php_mkdir_ex() which is a deprecated API from at least 17 years ago, and also fixes some of the return values
@Girgias Girgias marked this pull request as ready for review August 21, 2024 15:34
@Girgias Girgias requested a review from bukka as a code owner August 21, 2024 15:34
@bukka
Copy link
Member

bukka commented Aug 24, 2024

This looks actually worse than it was. I think it might be just better to undeprecate it and leave it as it is.

@bukka
Copy link
Member

bukka commented Aug 24, 2024

The thing is that php_stream_mkdir is not an exactly the same replacement and some extensions might use this. And the plain wrapper code was a bit more readable with php_mkdir...

@Girgias
Copy link
Member Author

Girgias commented Aug 24, 2024

The thing is that php_stream_mkdir is not an exactly the same replacement and some extensions might use this. And the plain wrapper code was a bit more readable with php_mkdir...

Nobody is using it: https://sourcegraph.com/search?q=context:global+php_mkdir+php_mkdir_ex+-f:file.h+-f:file.c+-f:plain_wrapper.c&patternType=keyword&sm=0

And arguably if you want to access the native functionality the correct thing to use VCWD_MKDIR()

@Girgias
Copy link
Member Author

Girgias commented Aug 24, 2024

One thing also is to try to remove some of the very strange interdependencies between main/Zend/PHP extension.

I don't think it is ideal for main (or Zend) to depend on PHP extensions.

If the only problem is readability, then I can extract it again as a static function in the same file.

@cmb69
Copy link
Member

cmb69 commented Sep 12, 2024

I don't think it is ideal for main (or Zend) to depend on PHP extensions.

This is a valid point, in my opinion. For that reason alone, I consider this a sensible change (although I wouldn't call a removal a refactoring).

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I missed that it's in ext/standard . Fine with it...

@Girgias Girgias merged commit 064ea9c into php:master Sep 22, 2024
8 of 10 checks passed
@Girgias Girgias deleted the outdated-api-change branch September 22, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants