Skip to content
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

Merge PHP5 and PHP7 code #16

Open
IonBazan opened this issue Jan 6, 2017 · 8 comments
Open

Merge PHP5 and PHP7 code #16

IonBazan opened this issue Jan 6, 2017 · 8 comments

Comments

@IonBazan
Copy link
Contributor

IonBazan commented Jan 6, 2017

I think managing PHP5 and PHP7 extension code in separate codebases is horrible. Can we try to merge these files to a single file?

@hnw
Copy link
Owner

hnw commented Jan 8, 2017

Thank you very much for your suggestion.

I think simply merging both files is more confusing. Because PHP API between PHP5 and PHP 7 are very different (hash API, zval, refcount, TSRM support, etc...), so some PHP extension project maintains 2 versions. (For example, php-memcached maintains version 3 for PHP7 and version 2 for PHP 5.)

Off course, increasing maintainability is important theme. So we should take another approach. For instance, PHP-independent common code should be splitted into library like php-timecop/tc_timeval.c.

@IonBazan
Copy link
Contributor Author

IonBazan commented Jan 8, 2017

@hnw I've tried to merge these files and found it nearly impossible because I don't have experience in writing PHP extensions. Spliting PHP-independent code would be a great idea.
I have one more question - does using TSRMLS_CC, TSRMLS_DC etc. is a bad practice in PHP7 extensions? As far as I know they are still available in PHP7 source for BC.

@hnw
Copy link
Owner

hnw commented Jan 11, 2017

I think the macros like TSRMLS_* should be removed in PHP 7 extensions. In PHP 7, we don't have to pass thread id explicitly in the thread safe mode.

See also: https://wiki.php.net/rfc/native-tls

Off course they are still harmless in PHP 7 (because it's defined as ""), so someone prefer to remain TSRM macros.

@IonBazan
Copy link
Contributor Author

@hnw Maybe we could add own macros implementing TSRMLS_* falling back to "" on PHP7 just for sure?

@hnw
Copy link
Owner

hnw commented Jan 25, 2017

No need to add any macros. It's already defined in TSRM/TSRM.h on PHP7.

/* BC only */
#define TSRMLS_D        void
#define TSRMLS_DC
#define TSRMLS_C
#define TSRMLS_CC

So, you can keep TSRMLS_* macros on your extensions if you want.

@francisbesset
Copy link
Contributor

The support of PHP 5.6 was ended the 19 Jan 2017.
The security support end the 31 Dec 2018.

I think that it is a bad idea to merge the 5 & 7.

@ruudk
Copy link

ruudk commented Nov 22, 2017

@francisbesset Indeed, just drop PHP 5 :)

@IonBazan
Copy link
Contributor Author

We can easily cut off PHP 5 but I believe this extension helps people writing tests for legacy code with no custom time provider. I think it is still worth supporting those people even after PHP5 support was dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants