During the DBTNG conversion (and this was not done by Berdir, alas) we got some superb ugly code in menu.inc. Begone.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
871 bytes

A more elegant patch thx DamZ.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Yay for code elegance.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Dries’s picture

Status: Needs work » Needs review

I'm undecided. First I thought #4 was uglier than #2. Then I decided it was just ugly to pass in a database result object as a parameter. ;-)

Other opinions?

Crell’s picture

Status: Needs review » Needs work

Well #2 just flat out doesn't work anyway because the API doesn't work like that. :-) #4 is an improvement over the current HEAD, but I agree with Dries that passing a database result object around is ugly in and of itself.

Also, the preferred way to specify the fetch mode is in the calling function (db_select() in this case), not on the result set after it's been retrieved. setFetchMode() is there because it's part of PDO, and it supports some more esoteric fetch options that as of yet I haven't found a use for but PDO allows. :-)

chx’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

Refactoring the whole shebang is not this issue. This is just removing some ugly code.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.11 KB

menu.admin.inc is already converted(!)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I still agree with Dries that passing around a result set is ugly. But the patch in #13 is an improvement over what's in HEAD right now, so RTBC.

quicksketch’s picture

I fix the passing around a database object in #483614: Better breadcrumbs for callbacks, dynamic paths, tabs. However, we can get this committed and then we can fix passing around the result set there, since I needed to rewrite menu_tree_data() there for other reasons.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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