[pLog-svn] r6503 -plugins/branches/lifetype-1.2/editcomments/class/action
Jon Daley
plogworld at jon.limedaley.com
Thu Jun 5 22:32:34 EDT 2008
The project hasn't been as fun as it was since Oscar left. I
don't really feel like I am working on a team, just me doing everything.
I have thought about leaving a number of times, and more about it today.
It wouldn't really change anything for me, ie. I would fork the project,
and continue developing as I am, but I wouldn't answer stuff on the
forums, so I would have more time to code, and less time discussing why
someone else thinks their way is better, on something that hardly matters,
and their method isn't any better than the current version.
I would have a whole lot less administrative work, and more fun
coding.
I am disappointed in the architecture, and how hard it is to
validate the inputs. It is very likely that whenever a new input is
added, it won't be validated appropriately. A number of our current
validators aren't very good, there are security holes all over the place.
I guess it is because lifetype is small enough so the spammers haven't
taken that much notice of it.
Questioning commits is fine, and that is a good thing. Lots of
bugs have been fixed by others watching the commits. In this case, your
solution at best isn't any better than mine, and as I have argued, is less
clear about what values are accepted than a simple regular expression that
any php developer can understand exactly how it works. Casting in most
programming languages is the wrong solution to almost every problem, and
while php does a better job than some languages, I wouldn't want to work
on any code that uses casts to do validation, that isn't what they are
made for.
Probably more frustration than anger, I think.
On Fri, 6 Jun 2008, Mark Wu wrote:
> Hi Jon:
>
> It seems you get angry yesterday.
>
> If you feel unhappy about the discussion, I am sorry about that.
>
> But, you have to understand, it is not a good way to show your anger in the
> mailing list.
>
> I think you remember you criticized or questioned my commits or others
> commits all the time.
>
> And I have to explain every details for you to let you know what I did in
> those commits. I just suppose you like this kind of discussions.
>
> If you think this kind of dicussion is not a good way to work in a team,
> maybe we should try to improve it, not just get angry.
>
> You know English is not my native language, so .. It may be not present my
> thoughts very well...
>
> If you feel bad with my wording or syntax, just let me know directly.
>
> Mark
>
>> -----Original Message-----
>> From: plog-svn-bounces at devel.lifetype.net
>> [mailto:plog-svn-bounces at devel.lifetype.net] On Behalf Of Jon Daley
>> Sent: Thursday, June 05, 2008 8:03 PM
>> To: LifeType Developer List
>> Subject: Re: [pLog-svn] r6503
>> -plugins/branches/lifetype-1.2/editcomments/class/action
>>
>> I really don't care. I don't know why you want to
>> include negative ids (which is what you get with a case), and
>> who knows what other values php might translate.
>> I am tired of you criticizing every single code
>> checkin. Feel free to do some real work.
>>
>> On Thu, 5 Jun 2008, Mark Wu wrote:
>>
>>> I think I know your problem now, but filteingr the value is not a
>>> proper way, here comes some thoughts:
>>>
>>> 1. if _op!= updateComments, we should use the comment list as error
>>> view 2. if _op== updateComments, we should check the
>> commentId exist
>>> or not, if yes, use the editCommentView, if not use the
>> comment list view.
>>> A better security check is we also check the blog id is in
>> comment is
>>> equal to the blog id in blogInfo or not..
>>>
>>> Mark
>>>
>>> ====================
>>>
>>> function AdminUpdateCommentAction($actionInfo, $request){
>>> $this->AdminAction($actionInfo, $request);
>>> $this->requirePermission( "update_comment" );
>>>
>>> $this->_op = $actionInfo->getActionParamValue();
>>> if($this->_request->getValue("cancel")){
>>> $this->_op = "cancel";
>>> }
>>>
>>> // articleId is needed, even on a cancel operation
>>> $this->registerFieldValidator("articleId", new
>> IntegerValidator());
>>> // should use a filter instead
>>> $this->_articleId = $this->_request->getValue( "articleId" );
>>>
>>> if($this->_op == "updateComment"){
>>> $this->registerFieldValidator("commentId", new
>>> IntegerValidator());
>>> $this->registerFieldValidator("commentText", new
>>> StringValidator(true));
>>> $this->registerFieldValidator("authorName", new
>>> StringValidator(false));
>>> $this->registerFieldValidator("commentStatus", new
>>> IntegerValidator());
>>> $this->registerFieldValidator("commentTopic", new
>>> StringValidator(false), true);
>>> $this->registerFieldValidator("authorEmail", new
>>> EmailValidator(), true);
>>> $this->registerFieldValidator("authorUrl", new
>>> HttpUrlValidator(), true);
>>> $this->registerFieldValidator("commentDateTime", new
>>> StringValidator(false));
>>> $this->registerFieldValidator("commentIp", new
>>> StringValidator(false));
>>>
>>> // should use a filter instead
>>> $this->_commentId = (int)
>>> $this->_request->getValue("commentId");
>>> $comments = new ArticleComments();
>>> if( $comments->getComment( $this->_commentId ) )
>>> {
>>> $view = new AdminEditCommentView($this->_blogInfo,
>>> $this->_commentId);
>>>
>>> $view->setErrorMessage(
>>> $this->_locale->tr("pluginEditCommentsInvalidData"));
>>> $this->setValidationErrorView( $view );
>>> $this->_fetchFields();
>>> } else {
>>> $view = new
>>> AdminArticleCommentsListView($this->_blogInfo,
>> array("article" => 0));
>>> $view->setErrorMessage(
>>> $this->_locale->tr("error_articleid_should_be_intege"));
>>> $this->setValidationErrorView( $view );
>>> }
>>> } else {
>>> $view = new
>> AdminArticleCommentsListView($this->_blogInfo,
>>> array("article" => 0));
>>> $view->setErrorMessage(
>>> $this->_locale->tr("error_articleid_should_be_intege"));
>>> $this->setValidationErrorView( $view );
>>> }
>>>
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: plog-svn-bounces at devel.lifetype.net
>>>> [mailto:plog-svn-bounces at devel.lifetype.net] On Behalf Of Jon Daley
>>>> Sent: Thursday, June 05, 2008 7:30 PM
>>>> To: LifeType Developer List
>>>> Subject: Re: [pLog-svn] r6503
>>>> -plugins/branches/lifetype-1.2/editcomments/class/action
>>>>
>>>> Please try it and see what happens, I don't know how to
>> explain it
>>>> to you.
>>>>
>>>> On Thu, 5 Jun 2008, Mark Wu wrote:
>>>>
>>>>> I don't think type casting is ambiguous, anyway, it is
>>>> personal choice.
>>>>>
>>>>> But, I still don't't get it... It is the code from
>>>>> adminupdatecommentaction.class.php
>>>>>
>>>>> // articleId is needed, even on a cancel operation
>>>>> $this->registerFieldValidator("articleId", new
>>>> IntegerValidator());
>>>>> // should use a filter instead
>>>>> $this->_articleId = preg_replace("/[^0-9]/", "",
>>>>> $this->_request->getValue( "articleId" ));
>>>>>
>>>>> If the the articleId is not an integer, why you need to
>> filter it??
>>>>> Our registerFiledValidator will reject it..
>>>>>
>>>>> You never have a chance to use it ....
>>>>>
>>>>> $this->_commentId has the same issues, if it reject by
>>>>> registerFiledValidator, you never have a chance to use it...
>>>>>
>>>>> Mark
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: plog-svn-bounces at devel.lifetype.net
>>>>>> [mailto:plog-svn-bounces at devel.lifetype.net] On Behalf
>> Of Jon Daley
>>>>>> Sent: Thursday, June 05, 2008 7:10 PM
>>>>>> To: LifeType Developer List
>>>>>> Subject: Re: [pLog-svn] r6503
>>>>>> -plugins/branches/lifetype-1.2/editcomments/class/action
>>>>>>
>>>>>> I won't ever use a cast to do that. preg_replace is
>>>> less ambiguous
>>>>>> about what characters are allowed.
>>>>>>
>>>>>> On Thu, 5 Jun 2008, Mark Wu wrote:
>>>>>>
>>>>>>> If it is a constructer, then use casting (int) articleId
>>>>>> will be better.
>>>>>>>
>>>>>>> Mark
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: plog-svn-bounces at devel.lifetype.net
>>>>>>>> [mailto:plog-svn-bounces at devel.lifetype.net] On Behalf
>>>> Of Jon Daley
>>>>>>>> Sent: Thursday, June 05, 2008 7:00 PM
>>>>>>>> To: LifeType Developer List
>>>>>>>> Subject: Re: [pLog-svn] r6503
>>>>>>>> -plugins/branches/lifetype-1.2/editcomments/class/action
>>>>>>>>
>>>>>>>> How do you suggest we reject it in the constructor?
>>>>>>>>
>>>>>>>> On Thu, 5 Jun 2008, Mark Wu wrote:
>>>>>>>>
>>>>>>>>> Why we need to filter it??
>>>>>>>>>
>>>>>>>>> If articleId is not integer, we should reject the request
>>>>>>>> instead of
>>>>>>>>> filter it....
>>>>>>>>>
>>>>>>>>> Mark
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: plog-svn-bounces at devel.lifetype.net
>>>>>>>>>> [mailto:plog-svn-bounces at devel.lifetype.net] On Behalf Of
>>>>>>>>>> jondaley at devel.lifetype.net
>>>>>>>>>> Sent: Thursday, June 05, 2008 6:52 PM
>>>>>>>>>> To: plog-svn at devel.lifetype.net
>>>>>>>>>> Subject: [pLog-svn] r6503
>>>>>>>>>> -plugins/branches/lifetype-1.2/editcomments/class/action
>>>>>>>>>>
>>>>>>>>>> Author: jondaley
>>>>>>>>>> Date: 2008-06-05 06:51:52 -0400 (Thu, 05 Jun 2008) New
>>>>>>>> Revision: 6503
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>>
>>>>>>>>>>
>> plugins/branches/lifetype-1.2/editcomments/class/action/adminu
>>>>>>>>>> pdatecommentaction.class.php
>>>>>>>>>> Log:
>>>>>>>>>> we need to manually filter the ids since we are grabbing
>>>>>>>> them in the
>>>>>>>>>> constructor. a 'real' Filter() would be better, but
>>>>>>>>>> 1.2 doesn't have very many filters available. We'll need
>>>>>>>> to add them
>>>>>>>>>> in 2.0
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>>
>> plugins/branches/lifetype-1.2/editcomments/class/action/adminu
>>>>>>>>>> pdatecommentaction.class.php
>>>>>>>>>>
>>>>>>
>> ===================================================================
>>>>>>>>>> ---
>>>>>>>>>>
>> plugins/branches/lifetype-1.2/editcomments/class/action/adminu
>>>>>>>>>> pdatecommentaction.class.php 2008-06-05 10:49:33 UTC
>>>>>>>> (rev 6502)
>>>>>>>>>> +++
>>>>>>>>>>
>> plugins/branches/lifetype-1.2/editcomments/class/action/adminu
>>>>>>>>>> pdatecommentaction.class.php 2008-06-05 10:51:52 UTC
>>>>>>>> (rev 6503)
>>>>>>>>>> @@ -36,7 +36,8 @@
>>>>>>>>>>
>>>>>>>>>> // articleId is needed, even on a
>> cancel operation
>>>>>>>>>> $this->registerFieldValidator("articleId", new
>>>>>>>>>> IntegerValidator());
>>>>>>>>>> - $this->_articleId = $this->_request->getValue(
>>>>>>>> "articleId" );
>>>>>>>>>> + // should use a filter instead
>>>>>>>>>> + $this->_articleId = preg_replace("/[^0-9]/", "",
>>>>>>>>>> + $this->_request->getValue( "articleId" ));
>>>>>>>>>>
>>>>>>>>>> if($this->_op == "updateComment"){
>>>>>>>>>> $this->registerFieldValidator("commentId", new
>>>>>>>>>> IntegerValidator()); @@ -48,8 +49,9 @@
>>>>>>>>>> $this->registerFieldValidator("authorUrl", new
>>>>>>>>>> HttpUrlValidator(), true);
>>>>>>>>>> $this->registerFieldValidator("commentDateTime",
>>>>>>>>>> new StringValidator(false));
>>>>>>>>>> $this->registerFieldValidator("commentIp", new
>>>>>>>>>> StringValidator(false));
>>>>>>>>>> -
>>>>>>>>>> - $this->_commentId =
>>>>>>>>>> $this->_request->getValue("commentId");
>>>>>>>>>> +
>>>>>>>>>> + // should use a filter instead
>>>>>>>>>> + $this->_commentId =
>> preg_replace("/[^0-9]/", "",
>>>>>>>>>> + $this->_request->getValue("commentId"));
>>>>>>>>>> $view = new
>>>>>>>>>> AdminEditCommentView($this->_blogInfo, $this->_commentId);
>>>>>>>>>>
>>>>>>>>>> $view->setErrorMessage(
>>>>>>>>>> $this->_locale->tr("pluginEditCommentsInvalidData"));
More information about the pLog-svn
mailing list