search

hnw / php-timecop

A PHP extension providing "time travel" capabilities inspired by ruby timecop gem
MIT License
385 stars 40 forks source link

Merge PHP5 and PHP7 code #16

Open IonBazan opened 7 years ago

IonBazan commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

hnw commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@francisbesset Indeed, just drop PHP 5 :)

IonBazan commented 7 years ago

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.