Re: Lowercase or camelcase function names?
| From: | Johannes | Date: | Mon, 07 Nov 2011 21:00:06 +0000 |
| Subject: | Re: Lowercase or camelcase function names? | ||
| References: | 1 2 3 4 | Groups: | php.pecl.dev |
| Request: | Send a blank email to pecl-dev+get-8819@lists.php.net to get a copy of this message | ||
Hello Johannes,
I have done some work on the extension today. It works and is cleaned
up a lot. Your hints where evry helpful to me. I have commented your
suggestions below.
On Mon, 07 Nov 2011 01:16:24 +0100
Johannes Schlüter <johannes@schlueters.de> wrote:
[...]
> Some small comments, without really deep into it:
> * pval isn't used anymore, I think it was even dropped with 5.3.
> zval is the exact same.
Fixed. No use of both of them anymore.
> * The PARSE_ARGS and ARG macros use an old dangerous, bad API.
> Look into using zend_parse_parameters (see
> README.PARAMETER_PARSING in php src)
> * The current code does ARG(0)->value.lval, this should be
> Z_LVAL_P(ARG(0)), while that call is, at least in
> scard_release_context, unsafe, zend_parse_parameters fixes that,
> though)
Both items fixed. I have changed the parameter handling code to use
"zend_parse_parameters". Macros removed.
> * Our CODING_STYLE asks for {} for every if clause etc. even when
> it's just a single line, the opening { should be on the same
> line as the "if" (or "while", "do", ..)
Fixed.
> * I assume there are some leaks, make sure to build PHP with
> --enable-debug
Done... 4 Leaks are still open... ;)
> * The CREDITS file should be two lines, first line extension name,
> second line author names. This is afaik used by the Windows
> build. It would also be used to create phpcredits() if this
> extension was to be bundled with PHP (which I don't expect to
> happen)
Done.
> * build.shlooks weird, phpize && ./configure && make should do all
> the things needed, else config.m4 is wrong.
Will check that. Just "phpize && ./configure && make" fails...
> * php_pcsc.h should be as sparse as possible, as for a static
> build it is included from main/builtin_functions.h which causes
> conflicts.
Cleaned it up... Am I missing something?
> * TODO: "Make the PCSC context a global var" - this sounds weird.
> Global variables often cause issues.
Don't know why I wanted it as a global variable when written this code
last year... :/ Line removed... ;)
> This long list might look worse than it is, the only first 3 items
> "matter" :-)
Yeah... I am on the right way... ;)
Should I do an account request in such a early state of the extension?
I really would like to provide it ASAP but I don't know if you like
0.1.0 versions at pecl.php.net. A good thing would be the use of PHP's
version control to keep changes at a central place...
I think the code is very stable and ready for use in non-production
environments. The PC/SC API is not implented completely. I removed a
lot of Johann Dantant's code to make it a little bit simpler. I am
willing to implement the complete API but at the moment the current
code can be used for server side smartcard development. I don't need
more function at the moment... ;)
I have needed this extension last year because many smart card terminals
are using Secure Access Modules (SAM) for doing communication to the
card on the client side. I have developed terminals for writing
smartcards online an needed this extension for communication with PHP
based web applications. Since JAVA, Python etc. does have well written
implementations I thought, since PHP is widespread, it would be nice to
do it for PHP as well...
The updated code is available at:
http://code.google.com/p/pcsc-for-php/
Regards,
Johannes
P.S.: I need to replace all my whitespaces with tabs... :/