Skip to content

[random] remove unnecessary old header files #9058

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

Closed
wants to merge 2 commits into from

Conversation

zeriyoshi
Copy link
Contributor

#8094

The Random Extension 5.x implementation kept the old header files to maintain extension compatibility.

  • ext/standard/php_lcg.h
  • ext/standard/php_rand.h
  • ext/standard/php_mt_rand.h
  • ext/standard/php_random.h

However, these files actually exist only to include php_random.h in the new ext/random and are no longer needed.

Deleting these files may affect some extensions, but this should not be a problem since it can be easily handled by branching with preprocessor macros.

#if PHP_VERSION_ID < 80200
# include "ext/standard/php_ABC.h"
# include "ext/standard/php_XYZ.h"
#else
# include "ext/random/php_random.h"
#endif

cc / @TimWolla @kocsismate

@TimWolla TimWolla requested a review from bukka July 20, 2022 07:49
@guilliamxavier
Copy link
Contributor

I'm a bit confused here, as @bukka (who asked the question) eventually "agreed" to the rationale to not remove them (https://github.com/php/php-src/pull/8094/files#r920702114) 😕 Wouldn't it be "better" to delay the removal to PHP 9.0? /cc @Danack (who requested to keep them)

@zeriyoshi
Copy link
Contributor Author

@guilliamxavier
Sorry, I was confused too. I suggest taking down this PR and removing it at 9.0.

@zeriyoshi zeriyoshi closed this Jul 20, 2022
@Danack
Copy link
Contributor

Danack commented Jul 20, 2022

Thanks. And yeah, batching up BC breaks until major versions, even if that means having a few 'dead' files around means less work overall for extension maintainers, probably.

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.

4 participants