New Plugin - One Voice…

Oh by the way you can also do this if you like :

{if already_spoken AND member_group !="1"}

in place of the above conditional in the first post and if you set this to your Super Admin group then Super Admins will always be allowed to post as many comments as they like.

Just thought that might help a bit too!! 😉

Best wishes,

Mark

Mark, this plugin has some pretty big problems. You’re using text straight from a parameter in a query without escaping it, are accessing an object property that might not be set, leading to PHP errors, are SELECTing * when you don’t use any fields, and you’re using ‘true’ as a variable name for a conditional. Lastly your usage shows that you can send member_id as a parameter, but that’s not the case. I highly suggest you read the developer guidelines, all three sections, which will be of tremendous assistance.

Hi Derek,

Thanks for the input. I did think last night that my conditional name was a bit silly so I am now changing this to {if already_spoken}, will that be better?

Regarding SELECT *, what is wrong with doing that as all I want to do with the plugin is check if there is a row for that user and that entry_id and if so don’t allow them to then comment. Is there a better way of doing this then?

Regarding the member_id=”“ parameter that was a mistook as it was originally in there and being used until I realised that I could just use the $SESS class instead to get what I needed. This has now been fixed.

The other two things you mention are the text parameter being used in a query. I am pretty sure I know what you mean by that as I guess that someone could paste anything into {segment_3} which could then lead to major problems. How would I go around this then or how do I sanitize (is that the correct word in this case) the parameter as a url_title could be anything really?

Is there a better way that I could go about making this plugin. I’d really like to learn the best way but just need some pointers if that’s okay? The developer documentation sometimes goes a little over my head and my coding skills aren’t all that good so any help you can give here would be greatly appreciated, thanks.

The last thing you mentioned was “accessing an object property that might not be set”. I must show my ignorance here as I don’t really know what that means? If you could possibly elaborate a little so that I can look this up or do whatever is needed then I will do. It’s not my intention to make plugins or what not that could be dangerous to use on sites so any help would be greatly appreciated.

Thanks in advance.

Best wishes,

Mark

Hi Mark

Just a quick note on the database thing: in all your custom db queries you should use

$DB->escape_str()

function for security reasons. Also if you only want to check if a row exists, instead of SELECT * selecting all fields (more resources intensive operation), you could also select only one field (as you’re only checking against an existing row) or even do a join and then use the DB functions to check the number of rows. Is that what was meant Derek?

Mark, read the developer guidelines, which include dos and don’ts code samples, and then return with questions that you need clarification on. Just about everything I mentioned is already covered in those, and you’ll benefit from the other information as well. The exception is this:

The last thing you mentioned was “accessing an object property that might not be set”. I must show my ignorance here as I don’t really know what that means?

Put this in a PHP enabled template, and don’t focus on the query, it’s just an example of a query that we know will return no results:

$query = $DB->query("SELECT entry_id FROM exp_weblog_titles WHERE entry_id != entry_id LIMIT 1");
$entry_id = $query->row['entry_id'];

@peschehimself - yes.

http://expressionengine.com/docs/development/guidelines/security.html#sql_injection_prevention

And on the query, selecting any fields at all is unnecessary since he isn’t using any of them.

SELECT COUNT(*) AS count...

Then of course you wouldn’t examine num_rows, as that will always return a result, an integer equal to the number of records returned in $query->row[‘count’].

Hiya,

Thanks for both chipping in on this. Derek I have read all the developer guidelines but at times they just go over my head. I have read everything you have both said and I am very grateful for the help on this as I would like to make this plugin (as simple as it is) work correctly and safely if I can.

I have attached a new version to the top post so perhaps you could let me know if I am now on the right lines? I think I have addressed most issues that were there in the older version?

Thanks for looking at this for me as I really do want to get better at all of this.

Best wishes,

Mark

Derek I have read all the developer guidelines but at times they just go over my head.

I feel you - I can imagine that for a less experienced coder those rules and guidelines can be pretty overwhelming, nevertheless it should not stop you from learning as you’re doing right now Mark.

$query = $DB->query("SELECT entry_id FROM exp_weblog_titles WHERE url_title = '".$DB->escape_str($url_title)."' AND weblog_id = '$weblog_id'");
$entry_id = $query->row['entry_id'];

This might still throw an error when no entry with that url_title in the weblog with weblog_id is found. You should also probably escape all variables passed to the DB query. Other than that, it looks good.

Hiya,

Thanks for the reply, this is very much appreciated. Thanks for the idea there. I hadn’t thought about if say someone places in the wrong weblog_id in the parameter. I was just taking it that they would do this right.

I have just uploaded V1.0.3 which hopefully now should be checking that if there are no results in the query that checks for an entry_id then it throws out the conditional error so that the form isn’t shown.

Hope I did this right?

Once again thanks for the help on this. I am trying my best to understand all of this, honest.

Best wishes,

Mark

Hi Mark

The problem is, if no row is found in your query (e.g. if someone sets the wrong url_title or weblog_id parameter), this line

if ($query->row['entry_id'] == "")

will throw an error, as there’s no ‘entry_id’ position in the row array. Instead you should use

if ($query->num_rows != 1)

or similar, as num_rows will be there in the $query object in any case and with this you actually check whether the query did find any rows in the first place.

Regards, Peter.

Ah right,

Actually I did originally have :

if ($query->row['entry_id'] <=0)

instead but I guess that was wrong as well. Will change it to what you have shown above. Thanks for the help on this.

Best wishes,

Mark

New version now uploaded to the top post.

Thanks again for the help on this. Hopefully now everything is accounted for? 😉

Best wishes,

Mark

Hi Mark

I don’t see any changes, in the code it still says:

if ($query->row['entry_id'] != 1)

Is the 1.0.3 version the latest?

Hi Mark I don’t see any changes, in the code it still says:

if ($query->row['entry_id'] != 1)

Is the 1.0.3 version the latest?

That was the change 😉

I used to have :

if ($query->row['entry_id'] == "")

Then I changed it to :

if ($query->row['entry_id'] != 1)

when you mentioned it. I probably changed it all a little too quickly 😉

Thanks for all the help on this, really appreciated.

Best wishes,

Mark