Skip to content

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