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

List all PHP extension api version no in zend_extensions.h #1523

Merged
merged 1 commit into from Oct 18, 2015

Conversation

c9s
Copy link
Contributor

@c9s c9s commented Sep 16, 2015

This PR provides a list of extension API no of each PHP release for extension developers to look up the extension versions

@KalleZ
Copy link
Member

KalleZ commented Sep 16, 2015

Minor detail here, I think they should be done at Zend/ level too, but as for the naming, then usually things in Zend/ are prefixed with ZEND_XXX, you can always make an alias of them with the name PHP_XXX from inside main/, for example main/php.h where the "PHP" counterpart of that value is defined.

@c9s
Copy link
Contributor Author

c9s commented Sep 16, 2015

Hi @KalleZ, do you mean to move the constants into main/php.h ?

@KalleZ
Copy link
Member

KalleZ commented Sep 16, 2015

Hi @c9s

I mean like, have the ones in Zend/ defined like:

#define ZEND_EXTENSION_API_NO_5_0                 220040412
#define ...

Then inside main/ we would have it like:

#define PHP_EXTENSION_API_NO_5_0 ZEND_EXTENSION_API_NO_5_0
#define ...

(Note I chacnged the names since it would sound 'wrong' otherwise)

This is usually the way many of the "cloned" API are named in regards to Zend/ and main/, cc @dstogov

@c9s
Copy link
Contributor Author

c9s commented Sep 16, 2015

makes sense, let me update the header files.

@c9s
Copy link
Contributor Author

c9s commented Sep 24, 2015

Hi @KalleZ ,

I only updated Zend/zend_extensions.h file because I think maintaining two constant list is too operose. One extension api version list shall be enough for extension development. :)

Thanks!

@wang-steven
Copy link

👍

@RickySu
Copy link

RickySu commented Sep 24, 2015

👍

2 similar comments
@killtw
Copy link

killtw commented Sep 24, 2015

👍

@Gasol
Copy link

Gasol commented Sep 24, 2015

👍

@nikic
Copy link
Member

nikic commented Sep 24, 2015

These look somewhat dangerous to me, since essentially the only thing you are doing here is exploiting that undefined macros are folded to 0 in #if and that #if ZEND_EXTENSION_API_NO < 0 is always false. If you tried using #if ZEND_EXTENSION_API_NO >= ZEND_EXTENSION_API_NO_5_5_X for example, it would not work correctly. The actual numbers you put into the macros are irrelevant.

So, if you want to add something like this, I'd drop the API nos and just define PHP_5_5_OR_NEWER macros, where usage is clear.

@c9s
Copy link
Contributor Author

c9s commented Sep 24, 2015

@nikic however if PHP_5_5_OR_NEWER is undefined, the #if is still false. And it won't work for php 5.5.x since the constants were defined 7.1.x.

I think this is just a re-useable constants & reference (or documented code) derived from ext/opcache/ZendAccelerator.h, the extension developers should be aware of the macro checking in their extension code.

@nikic
Copy link
Member

nikic commented Sep 25, 2015

@c9s You're right, that was a bad example. We could only start using this with 7_0_OR_NEWER etc, otherwise it wouldn't work.

So as this can only serve as documentation and cannot be used directly, maybe put the list of API nos in a comment instead?

c9s added a commit to c9s/php-src that referenced this pull request Sep 25, 2015
- Put doc constants in comment
- Link to github issue php#1523
c9s added a commit to c9s/php-src that referenced this pull request Sep 25, 2015
- Put doc constants in comment
- Link to github issue php#1523
@c9s
Copy link
Contributor Author

c9s commented Sep 25, 2015

@nikic I agree, the PR is updated.

- Put doc constants in comment
- Link to github issue php#1523
@c9s
Copy link
Contributor Author

c9s commented Sep 25, 2015

@nikic one idea: maybe we can have a extension skeleton generator that generates some helpful macros & functions in the header files. (maybe in another PR)

@c9s
Copy link
Contributor Author

c9s commented Oct 11, 2015

Hi @smalyshev, This PR is not a quickfix, It's adding document for extension version constants.

@php-pulls php-pulls merged commit 1273ae8 into php:master Oct 18, 2015
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.

None yet

9 participants