Skip to content

Conversation

hikari-no-yume
Copy link
Contributor

I think it's worth considering bumping the version number for ng. Changes may be yet to come, sure, but phpng was such a big overhaul I think it's worth calling "Zend Engine 3".

I haven't updated extensions which check for the constant, but here's a list from grep:

oa-res-27-90:php-src ajf$ grep -r ZEND_ENGINE_2 .
Binary file ./.git/objects/pack/pack-779abadf31d95ebd50611dbd5e7f4670f60dd109.pack matches
./ext/mysql/php_mysql.c:#ifdef ZEND_ENGINE_2
./ext/mysql/php_mysql.c:#ifdef ZEND_ENGINE_2
./ext/mysql/php_mysql.c:#ifdef ZEND_ENGINE_2
./ext/oci8/oci8.c:#ifdef ZEND_ENGINE_2
./ext/oci8/oci8.c:#else /* ZEND_ENGINE_2 */
./ext/oci8/oci8.c:#endif /* ZEND_ENGINE_2 */
./ext/pdo_firebird/firebird_driver.c:#ifdef ZEND_ENGINE_2
./ext/xml/xml.c:/* #ifdef ZEND_ENGINE_2
./ext/xmlwriter/php_xmlwriter.h:#ifndef ZEND_ENGINE_2
./ext/zip/zip_stream.c:#ifdef ZEND_ENGINE_2
./ext/zip/zip_stream.c:#endif /* ZEND_ENGINE_2 */
./main/internal_functions_win32.c:#ifndef ZEND_ENGINE_2

@laruence
Copy link
Member

the work is still undergoing...

@Sobak
Copy link
Contributor

Sobak commented Sep 20, 2014

@laruence: PHP 7 is also not finished yet :)

@smalyshev
Copy link
Contributor

I agree but I'd drop a note on the list about it to ensure everybody knows/nobody objects before merging this.

@hikari-no-yume
Copy link
Contributor Author

@hikari-no-yume
Copy link
Contributor Author

By the way, there might be some veiled version references in places, which would need changing. If someone spots any, let me know.


#define ZEND_ENGINE_2
#define ZEND_ENGINE_3
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a lot of code may do #ifdef ZEND_ENGINE_2, maybe it'd be good to leave ZEND_ENGINE_2 there but also add ZEND_ENGINE_3 in addition to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd make sense, would avoid updating existing extensions. Maybe we should add a comment for clarity in that case, though? Something like this:

/* ZEND_ENGINE_2 is also defined for backwards-compatibility
 * Therefore, do not assume that because ZEND_ENGINE_2 is defined, it is ZE2
 * This only guarantees it is ZE2 or later
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, actually, I don't think I'll keep ZEND_ENGINE_2 around. Code that checks for it should probably just be removed, since that's basically just checking for PHP 5, and it's been a decade since its release. Plus, this'd complicate checks by cross-5/7-compatible code, as ZEND_ENGINE_2 wouldn't guarantee it's actually ZE2, you'd have to do defined(ZEND_ENGINE_2) && !defined(ZEND_ENGINE_3), or worse, !defined(ZEND_ENGINE_3) which isn't future-proof against a hyptothetical Zend Engine 4.

@smalyshev
Copy link
Contributor

We may also want to change ZEND_EXTENSION_API_NO from 220140815 to 320140815

@hikari-no-yume
Copy link
Contributor Author

Good point.

@hikari-no-yume
Copy link
Contributor Author

OK, I updated the API number and I removed all references to ZEND_ENGINE_2.

@hikari-no-yume
Copy link
Contributor Author

FYI Travis should be green, it only failed because of Test if socket binds on 31338 [ext/sockets/tests/socket_create_listen.phpt].

@hikari-no-yume
Copy link
Contributor Author

I think this is ready to merge, but I'll let you be the judge. Stas?

@hikari-no-yume
Copy link
Contributor Author

Oh, I'd need to add to UPGRADING.INTERNALS, right?

@hikari-no-yume
Copy link
Contributor Author

Done.

@@ -29,10 +29,6 @@
#include <stdlib.h>
#include <stdio.h>

#ifndef ZEND_ENGINE_2
#error HEAD does not work with ZendEngine1 anymore
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do wonder why this is here at all. Were people trying to build PHP5 internal files for PHP 4? Was this in an extension? Regardless, I doubt we still need this message after more than ten years.

Copy link
Contributor

Choose a reason for hiding this comment

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

initially it came from 7c0e7b4#diff-89fb8b368c48bf62a149e0c2853141e7

and later on was moved to this location
914cf3c#diff-358b07727650bd64ec27ffb307a91f2f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, it's been 11 years, then!

@smalyshev
Copy link
Contributor

looks ok to me

@php-pulls php-pulls merged commit 8a065c5 into php:master Dec 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants