Editing a sticky node sets db field to -100 for non-weblinks nodes. Do not encode weight and remove the weight.inc file

In 7.23 new nodes get sticky = -100. I tick the checkbox to make them sticky and the db has +100. But if you edit the node again, the box is not ticked. Saving wihout making any changes sets the sticky value back to -100. Is this is desired behaviour? ie you always have to re-tick 'sticky' each time you edit the node?

Jonathan
ps could not select 7.23 from the drop-downs, but that's the version I am on.

I recently upgraded my site from 6.30 to 7.26. In 6.30 new/edited content receives a 0/1 in "sticky" field in node table. But in 7.26 now when I create a new content or edit the existing one it receives a -100/100 in "sticky" field and though the content is promoted to front page it is not displayed on front page because itz "sticky" field is set to -100 which should be 0.

Can anyone help with this issue??

I am having the same problem with my nodes in 7.26. All my custom content types, and the Basic Page and Article types will not keep the check if I check the sticky box, though checking it will toggle the value to the opposite of whatever it was, 100 or -100, in the database. I probably would never have noticed if it weren't that my theme (Marinelli 7.x-3.0-beta11) wants to flag everything as sticky, whether its value is 100 or -100. Meaning it puts a big red sticky image next to it.

The only nodes I have that do not behave this way are Weblinks. The only difference I can see is that the content type was created by the module, not be me or by Drupal at install. The sticky checkbox behaves as expected in node edit and the sticky flag appears, or not, as expected in my lists.

I am unable to reproduce this with Drupal core. Please supply steps to reproduce this issue with a clean install of 7.x.

My guess is that a contrib module or theme is changing the sticky value to 100/-100. Drupal core saves the value as 1/0. It's probably only looking for 1/0 when populating the node edit form, resulting in the issue where the sticky checkbox is unchecked when editing.

Hi dcam,

Yes, you are right. It works fine on a clean install, and in fact it is the Weblinks module (in my case) which is the cause of the unwanted behavior. Thanks kimble for the tip-off even if unintended. When I disable weblinks and edit a node, the sticky values are set to 0/1 in the db.

Weblinks works correctly on its own nodes, but I think a fix is required so that it does not do anything with other content types. Given that I have recently become a co-maintainer for Weblinks, I will work on this.

I would be interested to know if llaen or mmomin use Weblinks, and whether the problem goes away when you disable Weblinks.

Jonathan

This issue was already discussed in #2148005: weblinks.weight.inc stores all contributions in sticky 100 and -100.

The best would to remove this "feature" from Web Links and use the Weight Module instead.

Gerhard

I totally agree. I wish I had never added that feature.

OK. So we remove the hard-coded functionality, and re-write to integrate with the functions of the Weight module. If users want to use a weighting they need to install the weight modue. Is that how you see it working?

I would say yes.

Basically, the integration of the Weight module has already been added and implemented. Also, when removing the hardcoded weight stuff some tokens have to be removed as well.

Gerhard

Here's a first draft of a patch, against alpha1+6. Things done:

1. in .module, removed the module_load_include of weblinks.weight.inc
2. in .module, removed the node_weight item from the weblinks_form
3. in .module, removed the three other bits of code which dealt with weblinks own weight management.
4. in .admin.inc expanded the description of weblinks_page_sort to show if the Weight module is available and enabled for weblinks, and provide links to the project page and content type settings page
5. in .tokens.inc, removed two pieces of code which dealt with the node weight

Things to be discussed:

1. Do we need an update function in .install which un-does the 100/-100 values in node table sticky? If developers have already started using our alpha1 code to convert D6 sites they may have bad data which needs to be corrected
2. I have tried to delete the .weight.inc file in this patch by diff'ing to /dev/null but I'm not sure if that is the right way to do it

Thanks. I will test your patch in the next days.

Sure, we need an update function. Any D6 site using Web Links may have 100/-100 sticky values in the node table.

Regarding .weight.inc file: I have no idea how to delete a file from a module by a patch. But in fact it shouldn't bother since the file will be removed when a next version of the module will be installed.

I have now added an update function. From module.weight.inc it looks like a very straight-forward correction. Any negative values should be 0 and any values greater than 1 should be 1. The only danger is if there is another module which was also using the sticky field for secondary purposes and we overwrite those values. As we have found out, modules should not really be doing that, but I don't know how we can tell if the bad values were created by weblinks or not.

Anyway, here's the patch. For info, if you need to re-run the update again, reset the schema by executing:

db_update('system')->fields(array('schema_version' => 7000))->condition('type', 'module', '=')->condition('name', 'weblinks', '=')->execute()

[edit: Just noticed I have used the wrong issue number in the patch file name. But the patch is the correct one, and I'll fix it on the next re-roll]

The update function also needs to fix the node_revisions table. New patch attached (against latest dev alpha1+6)

The only danger is if there is another module which was also using the sticky field for secondary purposes and we overwrite those values.

And how about checking the content type of an affected node? Or checking if a matching entry does exist in the {weblinks} table? We should only modify Web Links related nodes.

We should only modify Web Links related nodes

No, that's the point. The weblinks weight code has been affecting all content types. They all need to be fixed.

Yes, you're right.

And what about checking for values which are not greater than 100 or less than -100?

Sticky should only be 1 or 0. Weblinks.weight converted 1 (sticky) into a +ve value upto 100, so any +ve value should be corrected back to 1. Likewise, weblinks.weight converted 0 (not sticky) into a -ve value from -100 to -1, so all of these should be set back to 0.

I think we have to do this - there is no other way to be sure we have fixed the errors our module caused.

I am about to re-roll the patch, for alpha1+8, unless you have any specific change you would like?

Re-rolled patch for new -dev after todays commits, alpha1+8

I've reviewed your patch. So far the patch looks working.

Do you see a chance to generate a list of affected nodes which would be modified by update #7002? Or how about displaying a hint how to generate such a list?

One other thing: the spelling of the module in the string assigned to variable $sort_description should be changed from 'Weblinks' to 'Web Links'. I thin 'Web Links' is the correct name.

If the Weight module is installed, you should either not change the value, or issue a message saying that this update will reset all the weights that have been set. I would prefer the former.

Patch tested and seems working well! But this patch is only for the 7.x-1.x-dev, not for the 7.x-1.0-alpha1, can you confirm?
Cheers, Francesco

Hi Francesco,
Yes the patch is for dev. The patch modifies five files - the changes in .module, .admin.inc and .tokens.inc all apply OK at alpha1, and the change to .weight.inc is actually deleting the file and this also works for alpha1. So you should be able to test the corrected functionality.

The only part of the patch which fails to apply is the update function for .install, because that part of the source file has since changed between alpha1 and dev. This was fixing the sticky values in the node table which were altered by Weblinks. If you know a bit of php you will see that in the patch I just added a new function weblinks_update_7002() which you can manually add at the end of your .install function. Or if you are unsure how to do that, and would like a new patch which can be applied cleanly to alpha1 let me know and I'll make one.

Jonathan

Just realised that my patch in #15 is exactly what you need if you want to apply it to 1.0-alpha1. That was rolled before the other recent change to .install so all parts apply and all five files are changed as required. You can then run the update - which will be called weblinks_update_7001() in this scenario.

Jonathan

@gerhard from #21

Do you see a chance to generate a list of affected nodes which would be modified

Here is some code which does this:

// Show which nodes will be corrected
$query = db_select('node', 'n')->fields('n', array('nid', 'type', 'sticky', 'title'));
$query->condition('n.sticky', 0, '<>')->condition('n.sticky', 1, '<>')->orderBy('nid', 'ASC');
foreach ($query->execute() as $sticky) {
  drupal_set_message(print_r((array)$sticky, TRUE));
}

We could add this into the readme.txt in a section 'Converting from 6.x to 7.x'

@nancy from #22

If the Weight module is installed, you should either not change the value, or issue a message saying that this update will reset all the weights that have been set.

We won't be altering any weights assigned via the weight module. This is only fixing the bad sticky values, created because the weight module is not available.

@francescosciamanna from #23

Patch tested and seems working well!

Thank you for testing.

I will change the text in the description, as per Gerhard's request in #21. Is there anything else to do before this is RTBC?

Good idea to add this piece of code into the README.txt file. An alternative would be to use a database tool like PHPMyAdmin or MSD to dump the affected nodes. This should be mentioned as well. However, the README file needs to be reviewed anyway.

Is there a way to select an update function interactively as it was possible in D6? Or can this be implemented to provide a mechanism to review and run the sticky corrections on demand?

One remark regarding your patch:

+++ b/weblinks.install
@@ -379,3 +379,34 @@ function weblinks_update_7001() {
+  return t('Corrected sticky nodes:@sticky, revisions:@sticky_revisions, Non-sticky nodes:@non_sticky, revisions:@non_sticky_revisions', array(
+    '@sticky' => $sticky,
+    '@sticky_revisions' => $sticky_revisions,
+    '@non_sticky' => $non_sticky,
+    '@non_sticky_revisions' => $non_sticky_revisions,
+  ));

Does this piece of code display the amount of corrected nodes, or just that corrections have been applied (1) or not (0). And shouldn't there be a blank character between the colon and the return value?

Apart from my above remarks I see no further things to be done.

If we provide the php in the readme file, anyone using phpMyAdmin should be able to understand the query and make their own. That's enough, I think.

Here is my test output using the php code before running the update:

Array ( [nid] => 5 [type] => page [sticky] => -100 [title] => Appellatio Esse )
Array ( [nid] => 94 [type] => weblinks [sticky] => 105 [title] => James number 94 )
Array ( [nid] => 95 [type] => weblinks [sticky] => -94 [title] => Henry link 95 )
Array ( [nid] => 103 [type] => page [sticky] => 100 [title] => Page for hidden nodes test )
Array ( [nid] => 111 [type] => article [sticky] => 100 [title] => Eum )
Array ( [nid] => 112 [type] => article [sticky] => -100 [title] => Enim Saluto Exim )
Array ( [nid] => 251 [type] => weblinks [sticky] => -103 [title] => Thomas the Tank Engine on Wiki (link 251) )
Array ( [nid] => 257 [type] => weblinks [sticky] => 92 [title] => Toby the Tram Engine ) 

When you run the update, the actual number of corrections are shown, as follows:

No, the queries are not optional in D7, you have to run them all.

OK, I agree.

But there should be also a short description about this specific update in the documentation block of the update function. And a database backup should be recommended before running the update.

The text in the documentation block for update functions is displayed during the update and the recommendation is only to have one line. I have expanded it with 'see readme.txt or weblinks.install for details' then put more details inside the actual function. Updated patch attached.

We do not need to recommend a db backup as that is always stated at the start of the update process.

I have added the php code from #26 into the readme.txt file. I think that is everything we need to do. I would like to get this committed then make a new release alpha2, so please take a look and if you are happy mark it RTBC.

Thanks

The added information in the readme.txt file is OK. I would only wrap some of the long lines at an earlier position.

One other thing: in the D7 version of Web Links I started to document significant changes in CHANGELOG.txt file. I think we should continue to do so. Thanks.

I've wrapped the long README.txt lines to 80 chars as for code. I've also added all the commits we did in 2014 to CHANGELOG.txt and separated out those in alpha1 from the those in the forthcoming alpha2. See commit 5b1a7e4 for details.

Thank you all for your help. It is very pleasing to get this committed.

Jonathan

Thanks. You're welcome.