Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
3.92 KB

And here we are.

Dries’s picture

This looks good. Looking at the tracker module test, we should have good text coverage for these changes. The one exception is that the tracker module tests don't test the paging part. For extra points, update the tracker tests to use a pager?

Dries’s picture

I've committed the patch in #1 to CVS. Thanks DamZ. We can talk more about tests and slave queries so not marking this 'fixed' yet.

Damien Tournoud’s picture

Ok, there was two mistakes:

- the object returned by the call to ->extend() should be used when calling ->execute()... if not, the extension as no effect at all
- tracker doesn't support tablesort (no header is associated with a sql column) which made tablesort.inc throw away some notices... a fix for which I sneaked in the attached patch.

moshe weitzman’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

I don't see the tag 'node_access' on the listing queries.

moshe weitzman’s picture

Ahem - still no 'node_access' tag. Lets not leave HEAD in an insecure state for months at a time.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

- Added the tag
- Re-ordered the query a bit, early extended and chained more calls..
- Removed tablesort extender, we don't need it..

Berdir’s picture

moshe suggested to remove the tests, no other page is testing the paging (except of the PagerDefault tests, obviously..).

New patch.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Ah, node access API thanks you.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks, Berdir.

Status: Fixed » Closed (fixed)
Issue tags: -DBTNG Conversion

Automatically closed -- issue fixed for 2 weeks with no activity.