Proposal for Web Services::Livejournal

Kitya Karlson (http://pear.php.net/
user/karlson) proposes Web Services::Livejournal.

You can find more detailed information here:
 http://pear.php.net/pepr/pepr-proposal-show.php?id=360

-- 
Sent by PEPr, the automatic proposal system at http://pear.php.net

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Justin Patrin (http://pear.php
.net/user/justinpatrin) has commented on the proposal
for Web Services::Livejournal.

Comment:

I have not checked through all of your code, but I have
enough comments for
this round. You have a lot to work on.

set_time_limit() shouldn't be called in the include file,
and especially
not just when it's included. You should instead put a
warning in the
documentation that tells people to make sure their time
limit is set
high.

Please use single quotes instead of double quotes for
strings:
http://pear.reve
rsefold.com/strings/

Lower case language constants are preferred (true, false).
	
There is some strange indentation in your files. Make sure
that you're
indenting with four spaces and no tabs. See the PEAR CS
section for more.

trigger_error is not allowed in PEAR packages. For PHP5
packages use
PEAR_Exception for all error handling.

Don't check the PHP version on runtime. The installer will
check for the
required version of PHP when the package is installed.

Don't use register_shutdown_function. It is especially
useless for
disconnecting from a database (or closing any resource) as
PHP will do
this at the end of the script anyway. If you want a
destructor, use
__destruct.

It would be helpful to allow the user to pass in their own
DB object (in
which case the destructor above would be harmful).

Please review your class and file structure. Class names
must map directly
to paths e.g. Services_LiveJournal_Common
Services/LiveJournal/Common.php.

You don't need & when setting an object to a variable,
such as with
$this->dbh = &DB::connect(). PHP5 uses references
automatically for all
objects.

There should alsways be spaces around operators such as =,
+, -, *, =>,
',', etc.

Long lines should be wrapped at around 40 chars.

Declare arrays before using them:
$data = array();
$data['response'] = '';

Declare arrays in one call instead of multiple. E.g.:

$data["response"]="";
$data["replyto"]=$replyto;
$data["parenttalkid"]=$parenttalkid;

Should be:

$data = array('response' => '',
	      'replyto' => $replyto,
	      'parenttalkid' => $parenttalkid);
	      
You don't need to unset() variables when they're no longer
in use,
especially when it it just at the end of a function. PHP
will clean up
local variables for you. The many unset() calls in your code
are
cluttering it up and are simply unneccesary in almost all
cases.

Use DB's quoteIdentifier() to quote table and field names
instead of
hard-coding the ` (only mysql knows `). Use quoteSmart()
instead of
hard-coding '.
e.g.:
'SELECT max('.$db->quoteIdentifier('id').
') FROM '.$db->quoteIdentifier('lj_comments').
' WHERE '.$db->quoteIdentifier('journal').
' =
'.$db->quoteSmart($this->_main->settings['usernam
e'])

When querying for a single row use getRow() instead of
query(),
fetchRow(), and free().

When querying for a single value use $db->getOne().

$comments_data[(string)intval($row[0])] is far too
convoluted. First,
(string) does nothing as intval() is creating an integer and
PHP treats
integer strings as integers when used as an array key.
$array = array();
$array['2'] = 1;
$array['02'] = 2;
foreach ($array as $key => $val) {
    echo var_dump($key);
}
Second, forcing the key as an integer here really serves no
purpose as
your DB table has the id field as an integer already and you
will get an
integer back.

You most certainly do not need to do this:
$comments_data[(string)intval($row[0])] =
array("parentid"=>$row[1],
"subject"=>$row[2],
"body"=>$row[3],
 "date"=>$row[4],
"status"=>$row[5],
"poster"=>$row[6]);
Use DB_FETCHMODE_ASSOC instead.

All output of variables into XML needs to be escaped unless
it was
previously escaped or sanitized. Use htmlentities() with
ENT_QUOTES for
attributes:
'<comment
level="'.htmlentities($comment['level'],
ENT_QUOTES).'">'
And use plain htmlentities() when text is not in an
attribute.
'<comment>'.htmlentities($text).'</comment>'

Private functions need full docblocks too.

Debugging would be better done through the Log package
instead of always
to STDERR IMHO.

Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=360

-- 
Sent by PEPr, the automatic proposal system at http://pear.php.net

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Thank you, these comments are most helpfull and will keep me
busy for a
few days trying to implement them.

Justin Patrin wrote:
> Justin Patrin (http://pear.php
.net/user/justinpatrin) has commented on the proposal
for Web Services::Livejournal.
> 
> Comment:
> 
> I have not checked through all of your code, but I have
enough comments for
> this round. You have a lot to work on.
> 
> 
> 
> set_time_limit() shouldn't be called in the include
file, and especially
> not just when it's included. You should instead put a
warning in the
> documentation that tells people to make sure their time
limit is set
> high.
> 
> 
> 
> Please use single quotes instead of double quotes for
strings:
> 
> http://pear.reve
rsefold.com/strings/
> 
> 
> 
> Lower case language constants are preferred (true,
false).
> 
> 	
> 
> There is some strange indentation in your files. Make
sure that you're
> indenting with four spaces and no tabs. See the PEAR CS
section for more.
> 
> 
> 
> trigger_error is not allowed in PEAR packages. For PHP5
packages use
> PEAR_Exception for all error handling.
> 
> 
> 
> Don't check the PHP version on runtime. The installer
will check for the
> required version of PHP when the package is installed.
> 
> 
> 
> Don't use register_shutdown_function. It is especially
useless for
> disconnecting from a database (or closing any resource)
as PHP will do
> this at the end of the script anyway. If you want a
destructor, use
> __destruct.
> 
> 
> 
> It would be helpful to allow the user to pass in their
own DB object (in
> which case the destructor above would be harmful).
> 
> 
> 
> Please review your class and file structure. Class
names must map directly
> to paths e.g. Services_LiveJournal_Common
Services/LiveJournal/Common.php.
> 
> 
> 
> You don't need & when setting an object to a
variable, such as with
> $this->dbh = &DB::connect(). PHP5 uses
references automatically for all
> objects.
> 
> 
> 
> There should alsways be spaces around operators such as
=, +, -, *, =>,
> ',', etc.
> 
> 
> 
> Long lines should be wrapped at around 40 chars.
> 
> 
> 
> Declare arrays before using them:
> 
> $data = array();
> 
> $data['response'] = '';
> 
> 
> 
> Declare arrays in one call instead of multiple. E.g.:
> 
> 
> 
> $data["response"]="";
> 
> $data["replyto"]=$replyto;
> 
> $data["parenttalkid"]=$parenttalkid;
> 
> 
> 
> Should be:
> 
> 
> 
> $data = array('response' => '',
> 
> 	      'replyto' => $replyto,
> 
> 	      'parenttalkid' => $parenttalkid);
> 
> 	      
> 
> You don't need to unset() variables when they're no
longer in use,
> especially when it it just at the end of a function.
PHP will clean up
> local variables for you. The many unset() calls in your
code are
> cluttering it up and are simply unneccesary in almost
all cases.
> 
> 
> 
> Use DB's quoteIdentifier() to quote table and field
names instead of
> hard-coding the ` (only mysql knows `). Use
quoteSmart() instead of
> hard-coding '.
> 
> e.g.:
> 
> 'SELECT max('.$db->quoteIdentifier('id').
> 
> ') FROM '.$db->quoteIdentifier('lj_comments').
> 
> ' WHERE '.$db->quoteIdentifier('journal').
> 
> ' =
'.$db->quoteSmart($this->_main->settings['usernam
e'])
> 
> 
> 
> When querying for a single row use getRow() instead of
query(),
> fetchRow(), and free().
> 
> 
> 
> When querying for a single value use $db->getOne().
> 
> 
> 
> $comments_data[(string)intval($row[0])] is far too
convoluted. First,
> (string) does nothing as intval() is creating an
integer and PHP treats
> integer strings as integers when used as an array key.
> 
> $array = array();
> 
> $array['2'] = 1;
> 
> $array['02'] = 2;
> 
> foreach ($array as $key => $val) {
> 
>     echo var_dump($key);
> 
> }
> 
> Second, forcing the key as an integer here really
serves no purpose as
> your DB table has the id field as an integer already
and you will get an
> integer back.
> 
> 
> 
> You most certainly do not need to do this:
> 
> $comments_data[(string)intval($row[0])] =
array("parentid"=>$row[1],
> 
> "subject"=>$row[2],
> 
> "body"=>$row[3],
> 
>  "date"=>$row[4],
> 
> "status"=>$row[5],
> 
> "poster"=>$row[6]);
> 
> Use DB_FETCHMODE_ASSOC instead.
> 
> 
> 
> All output of variables into XML needs to be escaped
unless it was
> previously escaped or sanitized. Use htmlentities()
with ENT_QUOTES for
> attributes:
> 
> '<comment
level="'.htmlentities($comment['level'],
ENT_QUOTES).'">'
> 
> And use plain htmlentities() when text is not in an
attribute.
> 
>
'<comment>'.htmlentities($text).'</comment>'
> 
> 
> 
> Private functions need full docblocks too.
> 
> 
> 
> Debugging would be better done through the Log package
instead of always
> to STDERR IMHO.
> 
> Proposal information:
> http://pear.php.net/pepr/pepr-proposal-show.php?id=360
> 

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Justin Patrin wrote:

>Please use single quotes instead of double quotes for
strings:
>http://pear.reve
rsefold.com/strings/
>  
>
I found back this other bench (not related, it's searching
a string in a 
set)
http://toggg.com/pe
ar/search.html

I intend to compare some different flavours of php.
Regards
-- 
toggg

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Justin,

I have just updated the code. New code is linked here. I
have changed it
according to your comments.

http://pear.php.net/pepr/pepr-proposal-show.php?id=360

Can you check it out?

Nikita.

Justin Patrin wrote:
> Justin Patrin (http://pear.php
.net/user/justinpatrin) has commented on the proposal
for Web Services::Livejournal.
> 
> Comment:
> 
> I have not checked through all of your code, but I have
enough comments for
> this round. You have a lot to work on.
> 
> 
> 
> set_time_limit() shouldn't be called in the include
file, and especially
> not just when it's included. You should instead put a
warning in the
> documentation that tells people to make sure their time
limit is set
> high.
> 
> 
> 
> Please use single quotes instead of double quotes for
strings:
> 
> http://pear.reve
rsefold.com/strings/
> 
> 
> 
> Lower case language constants are preferred (true,
false).
> 
> 	
> 
> There is some strange indentation in your files. Make
sure that you're
> indenting with four spaces and no tabs. See the PEAR CS
section for more.
> 
> 
> 
> trigger_error is not allowed in PEAR packages. For PHP5
packages use
> PEAR_Exception for all error handling.
> 
> 
> 
> Don't check the PHP version on runtime. The installer
will check for the
> required version of PHP when the package is installed.
> 
> 
> 
> Don't use register_shutdown_function. It is especially
useless for
> disconnecting from a database (or closing any resource)
as PHP will do
> this at the end of the script anyway. If you want a
destructor, use
> __destruct.
> 
> 
> 
> It would be helpful to allow the user to pass in their
own DB object (in
> which case the destructor above would be harmful).
> 
> 
> 
> Please review your class and file structure. Class
names must map directly
> to paths e.g. Services_LiveJournal_Common
Services/LiveJournal/Common.php.
> 
> 
> 
> You don't need & when setting an object to a
variable, such as with
> $this->dbh = &DB::connect(). PHP5 uses
references automatically for all
> objects.
> 
> 
> 
> There should alsways be spaces around operators such as
=, +, -, *, =>,
> ',', etc.
> 
> 
> 
> Long lines should be wrapped at around 40 chars.
> 
> 
> 
> Declare arrays before using them:
> 
> $data = array();
> 
> $data['response'] = '';
> 
> 
> 
> Declare arrays in one call instead of multiple. E.g.:
> 
> 
> 
> $data["response"]="";
> 
> $data["replyto"]=$replyto;
> 
> $data["parenttalkid"]=$parenttalkid;
> 
> 
> 
> Should be:
> 
> 
> 
> $data = array('response' => '',
> 
> 	      'replyto' => $replyto,
> 
> 	      'parenttalkid' => $parenttalkid);
> 
> 	      
> 
> You don't need to unset() variables when they're no
longer in use,
> especially when it it just at the end of a function.
PHP will clean up
> local variables for you. The many unset() calls in your
code are
> cluttering it up and are simply unneccesary in almost
all cases.
> 
> 
> 
> Use DB's quoteIdentifier() to quote table and field
names instead of
> hard-coding the ` (only mysql knows `). Use
quoteSmart() instead of
> hard-coding '.
> 
> e.g.:
> 
> 'SELECT max('.$db->quoteIdentifier('id').
> 
> ') FROM '.$db->quoteIdentifier('lj_comments').
> 
> ' WHERE '.$db->quoteIdentifier('journal').
> 
> ' =
'.$db->quoteSmart($this->_main->settings['usernam
e'])
> 
> 
> 
> When querying for a single row use getRow() instead of
query(),
> fetchRow(), and free().
> 
> 
> 
> When querying for a single value use $db->getOne().
> 
> 
> 
> $comments_data[(string)intval($row[0])] is far too
convoluted. First,
> (string) does nothing as intval() is creating an
integer and PHP treats
> integer strings as integers when used as an array key.
> 
> $array = array();
> 
> $array['2'] = 1;
> 
> $array['02'] = 2;
> 
> foreach ($array as $key => $val) {
> 
>     echo var_dump($key);
> 
> }
> 
> Second, forcing the key as an integer here really
serves no purpose as
> your DB table has the id field as an integer already
and you will get an
> integer back.
> 
> 
> 
> You most certainly do not need to do this:
> 
> $comments_data[(string)intval($row[0])] =
array("parentid"=>$row[1],
> 
> "subject"=>$row[2],
> 
> "body"=>$row[3],
> 
>  "date"=>$row[4],
> 
> "status"=>$row[5],
> 
> "poster"=>$row[6]);
> 
> Use DB_FETCHMODE_ASSOC instead.
> 
> 
> 
> All output of variables into XML needs to be escaped
unless it was
> previously escaped or sanitized. Use htmlentities()
with ENT_QUOTES for
> attributes:
> 
> '<comment
level="'.htmlentities($comment['level'],
ENT_QUOTES).'">'
> 
> And use plain htmlentities() when text is not in an
attribute.
> 
>
'<comment>'.htmlentities($text).'</comment>'
> 
> 
> 
> Private functions need full docblocks too.
> 
> 
> 
> Debugging would be better done through the Log package
instead of always
> to STDERR IMHO.
> 
> Proposal information:
> http://pear.php.net/pepr/pepr-proposal-show.php?id=360
> 

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Kitya Karlson (http://pear.php.net/
user/karlson) has initiated the call for votes on Web
Services::Livejournal.

Please review the proposal and give your vote here:
http://pear.php.net/pepr/pepr-proposal-show.php?id=360

-- 
Sent by PEPr, the automatic proposal system at http://pear.php.net

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Justin Patrin (http://pear.php
.net/user/justinpatrin) has voted -1 on the proposal for
Web Services::Livejournal.

Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=360
Vote information:
http://pear.php.net/pepr/pepr-vote-sho
w.php?id=360&handle=justinpatrin

Comment:

You really should have waited for further comments. I
hadn't looked
closely at your code before as you had lots to change. I
have far too many
more comments to justify a +1 right now. One comment,
however large, really
isn't enough to justify a call for votes. I'm voting -1
for now as I'd like
to see the remainder of these problems fixed before a call
for votes.
Assuming this proposal fails, create a new proposal and wait
for more
comments before a call for votes. Ask on the PEAR-dev list
before you call
for votes and wait.

Yet again, I still have not looked at all of your code as I
found plenty
in the first 3 or 4 files to comment on.

Your indenting is still funky. Look at your source online.
Most of your
throw commands are not indented right, for example.

Instead of parsing an ini file internally use a static
property. This will
allow users to use an ini file, an ini file with sections
(for multiple
packages) or even a normal PHP array. Something like:

static public $options;

or

static private $_options; and
Services_LiveJournal::setOptions($arr);

Or, if the option info needs to be configurable
per-instance, leave the
options as a private var but allow the user to set the
options as an array
in the constructor instead of passing in a filename.

Put the userInfo from DB errors in the exception's message.
If you don't
include iserInfo it's far more complicated to figure out
what went wrong
most of the time. ($err->getUserInfo())

In array declarations (such as $_defaultSettings) put each
key/value pair
on its own line for readability.

Don't use array_pop(explode('_', $className)), use
substr($className,
strrpos($className, '_') + 1).

Don't use () with require_once.

I suggest using only _method_map and not _object_map. Use an
associative
array instead of two arrays and searching.
$this->_method_map[$method->name] $object.

You don't need this:
if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
    $slash = '\';
} else {
    $slash = '/';
} 
Use DIRECTORY_SEPARATOR instead.

I'm really not sure what the code after this comment is
for:
// two folders, one file back
It looks a little complicated for something as simple as
setting a data
folder. It looks like this is meant to get the folder where
configuration
files may be kept. This should not be done in this way. Use
the PEAR
installer's replacement tasks to replace something like
data_dir with
the PEAR data directory. Any "default" config
files should be put in this
folder.

In addition, if this is something which you want to be
configurable (it
seems like it should be) then you should allow its setting
in the
constructor and/or in the main options array.

array_shift(split('_',$name,2)) also seems a bit wrong to
me. User substr
and strpos.

I'm not sure I like the way that getConfigValue is reading
from a file
every time the config value may be used. Are these files
ever used
multiple times in one script run? If so, it may make more
sense to cache
the file contents.


Don't use escapeSimple(). Use quoteSmart() and remove the
hard-coded '
around the values.

ALWAYS escape output when putting it in HTML. Within an
attribute you need
to use htmlentities($val, ENT_QUOTES). Within normal HTML
just
htmlentities() is fine. (see _formatUserInfo()).

NOW() is a mysql-ism. If you leave your call as NOW() then
your code is
going to be tied to certain databases.

Again, $this->_memories[(string)$row[0]] really isn't
going to help. You
don't need the (string). You have these all over the place.

++$var is more efficient than $var++

Don't silence calls unless you have a compelling reason.
$this->_memories[(string)$id] is not a compelling
reason. Check for it
being set with isset() first.
constant('UTF-8') is also not a compelling reason.
If UTF-8 is not always
defined then use is_defined() first.

Don't use function_exists($callback), use
is_callable($callback). This
allows for method callbacks.

Use DB's auto* functions more often instead of hard-coding
so much simple
SQL.
http://pear.php.net/manual/en/package.database.d
b.intro-auto.php
In addition, prepared statements may save you some runtime
if you have to
run the same SQL over and over.

-- 
Sent by PEPr, the automatic proposal system at http://pear.php.net

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

PEPr has automatically extended the voting time on Web
Services::Livejournal until 2006-03-22 13:02 UTC because
there were not enough votes, yet.

Please review the proposal and give your vote here:
http://pear.php.net/pepr/pepr-proposal-show.php?id=360

Voting time is extended only once per proposal.

-- 
Sent by PEPr, the automatic proposal system at http://pear.php.net

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

PEPr has automatically finished the proposal on Web
Services::Livejournal.

Sum of Votes: -2 (0 conditional)
Result:       This proposal was rejected

Further details on the status of the proposal and the votes
can be found here:
http://pear.php.net/pepr/pepr-proposal-show.php?id=360

If you are the person who initiated the proposal,
please read the 'Proposal Finished' instructions at
http://pear.php.net/manu
al/en/newmaint.proposal.step3.php#newmaint.proposal.step3.st
age4

-- 
Sent by PEPr, the automatic proposal system at http://pear.php.net

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php