I have setup a sub module to implement hook_pathauto for the Classified Ads categories.

The Patch to add this to ed_classified is attached.

Can the patch be reviewed and possibly be committed?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fgm’s picture

Status: Needs review » Needs work

The idea is interesting, however, I wouldn't include it right now:

  • BUG (Major): The code is using pathauto code without declaring a dependency on it
  • BUG: The code implementing hook_pathauto() should check $op: the only handled value is settings so any other value, presumably added by a third-party, must be ignored.
  • BUG: The array values in $settings->patternitems must be translated
  • BUG: The info file is missing package = Classified
  • BUG (Performance): Code loads term entities individually using taxonomy_term_load(), but it should be using a load_multiple instead, via taxonomy_term_load_multiple(), to reduce server load
  • BUG (minor): There are a number of Drupal coding standards violations, mostly regarding spacing and indenting.
  • Style: It is using direct SQL access to handle the taxonomy terms. However these are entities, so you should really be using EntityFieldQuery instead of the lower-level DBTNG
  • Quality: There are no tests: functionality should be covered by integration tests (Simpletest) and/or unit tests. If you use unit tests, I would accept PHPunit since we're using them in D8 now
inventlogic’s picture

Status: Needs work » Needs review
FileSize
11.61 KB

Worked my way through the review points above and edited and added to cover all.

Please can you review again.

fgm’s picture

Status: Needs review » Needs work

Lots of things to review, thanks.

However, I notice that you used Upper_Underscored function names in the module file (and name): please do not do this, but always use lowercase for non-OO code and module names. See https://drupal.org/coding-standards#naming for more details.

ClassifiedCategoriesUrlTestHelper derives from the webtestcase, so these are integration tests, not unit tests (just a commenting issue).

Also, beware of your text editor: it leaves spaces at the end of some lines.

I'll take more time to actually review it soon, but could you please change this already ?

inventlogic’s picture

Amended as per your suggestions.

There are 5 fails in the tests for Bulk Update that I cannot figure out at all.

Could you let me know why they are failing?

inventlogic’s picture

Status: Needs work » Needs review

fgm’s picture

Status: Needs review » Closed (won't fix)

With this module end of life approaching along with Drupal 7 EOL, this issue is now moot.