Status discrimination !

guiguibonbon · 2007-09-06 17:39:25

Sticky articles require two queries for no apparent reason! Where’s this world going : will black people have to pay their bus tickets twice now?

Ok, seriously though, the first query is called in pretext(), the second one in doArticle(). Maybe I’m missing something here, but I don’t see any reason for this.

line 418 should be :

$a = safe_row('*, unix_timestamp(Posted) as uPosted', 'textpattern', 'ID='.intval($id).' and Status >= 4');

line 771 should be :

if (!in_list(array(4,5,"live","sticky"), $status) or empty($thisarticle) or $thisarticle['thisid'] != $id) {

Also, while txp:article allows you to list articles with a status other than sticky or live, pretext triggers a 404 error when you try too see one on its own page. It’s debatable of course who does what wrong here (although I think the user should be able to choose, therefore I think draft articles should be displayable if you ask for it). Which means…

lookupByTitle() and the like shouldn’t check for status in their query.
Neither should line 418 (or is there some security threat i’m overseeing here- e.g. would it be possible to maliciously access $this_article outside an article tag – while it would contain a draft article ? Through comments or something?)
the 404 error should be triggered by doArticle() if status doesn’t match. some ob_clean() should allow that, no?

One last thing : couldn’t we save one more query by having lookupByTitle() and its brothers do populateArticleData() in case of success? The only downside I see to this is a small increase in query-load when one types a wrong id or section. Still, I think overall it’s less resource-intensive, and isn’t it more important to have article pages load a tiny faster than having 404 pages load a tiny faster?

ruud · 2007-09-06 20:57:21

I’ve already seen that one and am working on a fix (btw, the problem doesn’t just affect stickies, but live articles as well.

As I said before, you’re not supposed to use anything other than live/sticky. Most of the TXP code is based on that very valid assumption. The other statuses are not for public articles.

guiguibonbon · 2007-09-06 21:40:43

ruud wrote:

I’ve already seen that one and am working on a fix

cool!

btw, the problem doesn’t just affect stickies, but live articles as well.

er… actually, I’m pretty shure it doesn’t. That’s what this line is avoiding, right? Or were you referring to the fact lookupByTitle() and its brothers should do populateArticleData() ? In fact live articles require two queries for no reason, but stickies get three.

As I said before, you’re not supposed to use anything other than live/sticky.

It’s definitely questionable.

Most of the TXP code is based on that very valid assumption.

Sorry to be cocky here, but from my experience with fooling around with it, no, it doesn’t. Textpattern works just as expected when you allow articles other than sticky or live to be displayed. All that has to be changed is what I said there. This means the question is simply a matter of ethics : is it advisable to give people the freedom to do this?

That’s where a good Dutchman usually says yes :D

ruud · 2007-09-07 23:08:51

er… actually, I’m pretty shure it doesn’t. That’s what this line is avoiding, right?

Nope. Since $status mostly evaluates to TRUE, that condition triggered for both live and sticky articles, causing the extra query. Should be fixed in SVN revision 2636

Or were you referring to the fact lookupByTitle() and its brothers should do populateArticleData()?

I haven’t looked at that part yet. Compared to 4.0.5 the number of queries is already reduced by two.

is it advisable to give people the freedom to do this?
That’s where a good Dutchman usually says yes :D

Your freedom is in no way restricted by this. That’s one of the benefits of using software released under the GNU GPL license :)

guiguibonbon · 2007-09-07 23:26:27

That’s one of the benefits of using software released under the GNU GPL license :)

Damn, should have seen that one coming.

Great work though :) Thanks.

Nope. Since $status mostly evaluates to TRUE, …

right, default attribute value, me being stupid again :)

guiguibonbon · 2007-09-09 20:52:02

Or were you referring to the fact lookupByTitle() and its brothers should do populateArticleData()?

I haven’t looked at that part yet. Compared to 4.0.5 the number of queries is already reduced by two.

I had this discussion with Mary about how nice it would be to have a fetch_image function in the core, that keeps the requested data in a static array. I think that would be nice with articles too. This way, lookupByTitle(), lookupByIdSection(), etc. would call that function but do nothing with the results; and when populateArticleData() calls the function again, it returns the results of the previous query.

ruud · 2007-09-09 21:11:50

For 4.0.x that’s too big a change, IMHO

Textpattern CMS

Textpattern CMS support forum

#1 2007-09-06 17:39:25

Status discrimination !

#2 2007-09-06 20:57:21

Re: Status discrimination !

#3 2007-09-06 21:40:43

Re: Status discrimination !

#4 2007-09-07 23:08:51

Re: Status discrimination !

#5 2007-09-07 23:26:27

Re: Status discrimination !

#6 2007-09-09 20:52:02

Re: Status discrimination !

#7 2007-09-09 21:11:50

Re: Status discrimination !

Board footer