Sort by weight using Weight module

Thanks for reporting this.

It seems that the weight module no longer relies on the sticky attribute of a node for storing its weight:

Yes, that does not work anymore under D7.

the _weblinks_get_query() function should be updated to reflect the changes in the weight module.

I think not. The node ordering should be now fully controlled by the Weight Module.

Some things to check:

• Did you read the documentation of the Weight Module at https://www.drupal.org/node/307797?
• Did you enable Weight for content type Web Links?
• Did you check the Weight Module settings?

As of current Jonathan and I are on vacation. Therefore working on this issue may take some time.

Gerhard

Yes, I checked the things you mentioned in #2.

OK. Then I assume that you see a 'Weight' field per Web Links node. See you any input in the 'Weight' field in the weight_weights table?

Hi Martin,
I've looked at this further, and yes you are right that we might need to alter _weblinks_get_query(). In the conversion of weblinks from D6 to D7 and the subsequent removal of the weight functionality in #2030765: Editing a sticky node sets db field to -100 for non-weblinks nodes. Do not encode weight and remove the weight.inc file means that ordering by weight is not currently supported. I had noticed that the sort options we provided in D6 may not apply in D7, but I'd not done any more testing.

I think we need three options in the radio selection (1) sort by title, (2) date and (3) by weight. In all cases, the sticky attribute will be put at the top of the order, so the original purpose of 'sticky' will be retained.

I'll work on a patch for this and get something up here for you to test.

Jonathan

Here's a first patch. I have left in the old code for comparison. To avoid duplication of code for the main page sort and group sort I have created a helper function. Comment in the code should explain it.

Have a try and let me know how it goes. I have left in some devel dd() calls which can be uncommented if you wish.

Thanks. I have tested your patch and found one deviation:

+++ b/weblinks.module
@@ -1635,6 +1646,11 @@ function _weblinks_get_query($tid = 0, $sort = 'title', $limit = 0) {
+    case 'weight':
+      $query->orderBy('sticky', 'DESC');
+      $query->orderBy('weight');
+      $query->orderBy('created', 'DESC');
+      break;

The last orderBy should use column 'title' according to your description.

Second, there are some minor spelling errors: 'thier' should be 'their' and 'mian' should be 'main'.

I have left in the old code for comparison.

OK. But I assume you will remove the old code in the final version of the patch. Since the old fieldset will be obsolete later and causes extra load. At least on my test site opening the Web Links Settings page causes a "Script not responding error" in Firefox.

Have a try and let me know how it goes.

Apart from the above described deviation the patch works.

I have left in some devel dd() calls which can be uncommented if you wish.

I have not used them. But what about using a Define or something similar to enable/disable the dd() calls?

Thanks for the review. Yes, I thought the patch would need a few things to be tidied up but I rushed it out so you and Martin could test earlier rather than later. I'd spotted the typos (one of which was in the old code not mine) and I had noted a question to ask regarding the sort order for 'weighted'. Given that the Drupal default order is by descending date I think that the final field should be date, ie 'sticky, weight, descending date'. So I will change the description for that.

I've also reverted in #524686: Node revision revert not handled a couple of lines that had got committed early. So you'll now have to download 1.0+7-dev to test this new patch. But it makes it cleaner. Yes I will definitely remove the old code for final version. My Firefox does not cause that 'script stopped' error, so I am not sure if there is anything wrong.

In .css to grey-out the disabled radio buttons I removed the unnecessary class 'weblinks-disabled' and changed it to use the standard 'form-disabled' class.

what about using a Define or something similar to enable/disable the dd() calls?

OK, I've left the debug in and uncommented, but created a flag for whether the devel module exists. This should mean that the code will still pass D.O. automated testing (which does not have the devel module)

Thanks for the review.

You're welcome.

I'd spotted the typos

OK. But there is still one: 'thier'.

The patch works, but there a some other things to be adjusted:

• The spelling of 'weight module' is not consistent: in my opinion it should be 'Weight module'
• The description of the link to the Web Links content type should be 'Web Links content type'

Drupal default order ... So I will change the description for that.

OK.

My Firefox does not cause that 'script stopped' error, so I am not sure if there is anything wrong.

I think it's just due to the complexity of the settings page and the used jQuery scripts.

but created a flag for whether the devel module exists.

Great, thanks.

Thanks for the useful replies.

1. The typo 'thier' is in the existing code, which is going to be deleted, so I did not need to fix that.
2. Yes, I agree it should be 'Weight module'. Done
3. The description of the link ... should be 'Web Links content type'. Done
4. Use @placeholder instead of !placeholder. Thanks for the suggestion but I'm not sure it is actually needed here as the text is all provided by strings in the code, not user input, so we know it contains no nasties. I have changed them anyway :-)

Here is a clean patch, with no debug, with the old code removed.

I have tested the patch and it works.

But before marking it RTBC I have one question:

+++ b/weblinks.admin.inc
@@ -160,36 +159,21 @@ function weblinks_admin_settings() {
+    '#description' => $sort['description'],

In function weblinks_form_alter you have used filter_xss to sanitize the description of the fieldset and here not. Any specific reason for the different implmentation?

The typo 'thier' is in the existing code, which is going to be deleted, so I did not need to fix that.

You're right. Sorry, I missed that.

Good spot! The reason I added it was to avoid the critical(!) warning produced by Coder Review:

But there was no warning produced for the one in weblinks_admin_settings() so I forgot that I'd treated them differently. I do not know the logic behind the review coding - maybe the fact one function is the standard hook_form_alter() makes the difference.

In fact, there is no real need to use filter_xss here because the text is provided in the module, not generated by user input. But I do not want to introduce new warnings in Coder after all the work in #2396277: Coder review and cleanup for coding standards. It may be inconsistent, but I don't think we should add to the processing overhead unless there is a reason (either real of for cleaner review results). Is that OK?

Is that OK?

Yes, that is OK. I was just going to be sure to have that checked.

Thank you Martin for raising this issue, and thank you Gerhard for your eagle-eyed reviewing and testing.

I had tested what happens when the sort option is 'weight' and then the Weight module is disabled, but I did not report the findings above. Just for the record, it is all OK. The weight table is still available internally for sql queries even if the field is not displayed for edit, so until the admin changes the sort option the links continue to be sorted by the existing weights, and no errors are produced. So that's good.

Thanks.

I had tested what happens when ...

That's OK for me as well. When an admin disables the Weight module he/she must also take care about any dependencies. It might be more consequent to disable weighted ordering already when the Weight module got disabled. But for the time being we can leave at is implemented right now.

It might be more consequent to disable weighted ordering already when the Weight module got disabled.

The radio button is disabled and not clickable when the Weight module is not available, but I decided against changing the sort setting, for two reasons (a) there is no obvious choice for what to change the sort setting to, (b) doing things automatically can cause more confusion when the admin should actually see the situation and make their own choice.