External blog posts not removed from {post} table when the blog is unregistered

Triaged as suggested by Eloy. Thanks Andrew.

I would also suggest that the person who registered the external blog, be able to delete individual posts from the registered blog as well.

Update--I see this has been reported in MDL-27427

This is a huge problem, I registered a blog thread and I cannot as full admin delete these posts. This is ridiculous, and now I had to change the setting to hide blogs from each user because they flooded the student blogs area. Please fix this issue, for 2.0 and up.

I had to remove my account and create a new one as a quick fix to remove all the blog posts that were associated with my account. This is the best we can do on the front end in the meantime.

Adding a branch with a solution. I'll create the other branches after peer review. This took a bit longer than I expected. We're storing a row ID in a text column.

Looks good to me, the usage of sql_compare_text() looks messy, but I can't see a way around the problem

Adding branches for 2.3 and 2.2 and putting this up for integration.

Wow, this is completely screwy isn't it!

1. Understandably (as its confusing) the use of sql_compare_text in the second case isn't quite right. Instead you should do $DB->sql_compare_text('content') . ' = ' . $DB->sql_compare_text('?') and then pass $delete as a parameter along with userid and module. The way that its written at the moment will put the parameter directly in the SQL rather than using parameters correctly.
2. I notice that other places looking for external blogs check for 'uniqueid' being set. Should a $DB->sql_isnotempty() be put on uniqueid to ensure this condition is met to be consistent?

Reopening for the first point, as it represents and sql injection risk.

I believe I've fixed both of those issues. I have added the not empty check for uniquehash. Its an odd field and Im not 100% sure of its role however we're better off incorrectly leaving old blog posts in the database rather than accidentally deleting something we should keep.

I've updated the master version. I'll be away for a few hours. If this gets integrated in the mean time it should be able to be cherry picked cleanly.

the

' AND '.

line could use cleaning up, moving the first ' to the end of the line above, other than that, it seems to fix the issues Dan raised

Submitting for integration.

Dear integrators, I'm having some git trouble so couldn't create a 2.3 and 2.2 version. It should however be cherry-pickable.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Oh my, using text for storing those ids.. We really need to move the posts table to use the component, area, itemid behavior ASAP.

The main question here is what happens with already orphaned posts, shouldn't we be deleting them (aka, upgrade step?). Else the bad behavior will continue happening, isn't it?

I don't think this can land without them getting fixed (deleted). So reopening, sorry.

Ciao

I've added a second commit containing upgrade code to clear out the orphan external blog posts. Putting this up for peer review.

After a chat with Eloy Ive removed the NOT IN due to concerns about performance on large datasets. However, for the life of me, I cannot get this query to work. Not sure what Im doing wrong.

Hi Andrew. Sorry, I did not dive too deep into this - but this caught my eyes:

WHERE b.id = p.content

at least, it seems like integers are compared with a text field here, no? Can it be the reason? And what error do you get?

Hello David,

I thought the same, but seems it is correct. For external blogs we keep external_blog.id in post.content.

@Andrew: It seems like a problem with post alias. Can you try p.uniquehash <> '' instead of "$DB->sql_isnotempty('post', 'uniquehash', false, false);"

Still no luck. The error Im getting is below.

Debug info: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'p
WHERE mdl_post.module = 'blog_external'
' at line 1
DELETE FROM mdl_post p
WHERE mdl_post.module = ?
AND NOT EXISTS (SELECT b.id FROM mdl_blog_external b where b.id = p.content)
AND p <> ""
[array (
0 => 'blog_external',
)]
Error code: dmlwriteexception
Stack trace:

    line 410 of /lib/dml/moodle_database.php: dml_write_exception thrown
    line 860 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
    line 1129 of /lib/db/upgrade.php: call to mysqli_native_moodle_database->execute()
    line 1483 of /lib/upgradelib.php: call to xmldb_main_upgrade()
    line 271 of /admin/index.php: call to upgrade_core()

Update: I've now got a working query. Its a bit odd but aliasing the post table seemed to break it for reasons I don't quite understand. If anyone has a better working alternative by all means post it. Please try it before posting as whatever idea first pop into your head I've probably already tried it during what has been a rather frustrating morning.

Looks fine to me Andrew

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

thanks for the many reviews and iterations here.

i've cherry-picked these 2 commits to 22 and 23. its in integration master too.

Andrew: perhaps the sql issue you're encountering could be a MDL issue to help improve the dml API.

the upgrade bit is giving me this under postgres:

Debug info: ERROR: operator does not exist: bigint = text
LINE 3: ... (SELECT b.id FROM mdl_blog_external b where b.id = mdl_post...
^
HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts.
DELETE FROM mdl_post
WHERE mdl_post.module = 'blog_external'
AND NOT EXISTS (SELECT b.id FROM mdl_blog_external b where b.id = mdl_post.content)
AND ( NOT (uniquehash = '') )
[array (
)]
Error code: dmlwriteexception
Stack trace:
line 410 of /lib/dml/moodle_database.php: dml_write_exception thrown
line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
line 631 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
line 1193 of /lib/db/upgrade.php: call to pgsql_native_moodle_database->execute()
line 1483 of /lib/upgradelib.php: call to xmldb_main_upgrade()
line 271 of /admin/index.php: call to upgrade_core()

note: its fine for mysql and mssql for me.

trying with sql_cast_char2int()

yeah, sql_compare_text() is to be able to compare varchar & text columns, not ints at all, for that, explicit cast is needed for sure.

offtopic, just looked for sql_compare_text() ocurrences in /blog and it's plenty of "incorrect" ones. Practically all should be casting ones. We really need to move those integers out from that column asap, grrr.

Anyway, to get this fixed... we need to do something like this:

index 7447298..5c4d234 100644
--- a/lib/db/upgrade.php
+++ b/lib/db/upgrade.php
@@ -1185,7 +1185,7 @@ function xmldb_main_upgrade($oldversion) {
     }
 
     if ($oldversion < 2012090400.00) {
-        $subquery = 'SELECT b.id FROM {blog_external} b where ' . $DB->sql_compare_text('b.id') . ' = ' . $DB->sql_compare_text('{post}.content');
+        $subquery = 'SELECT b.id FROM {blog_external} b where b.id = ' . $DB->sql_cast_char2int('{post}.content', true);
         $sql = 'DELETE FROM {post}
                       WHERE {post}.module = \'blog_external\'
                             AND NOT EXISTS (' . $subquery . ')

and cross the fingers about not finding non-integer values in that "contents" column or it will be breaking badly too.

failing coz it's clear it does not work under, at least, postgres.

ok i've pushed up a commit into 22, 23 and master in integration.git.

mssql, pg and mysql fine here atm. (upgrades)

Just noting that the old code broke on Oracle, but it works after Apu's fix.

This was during an upgrade on another issue. I wasn't specifically testing this issue.

I've now tested this under Oracle and MSSQL.

Thank you Michael for your help testing this issue.

I tested on mysql and postgres, both DB works great.

Test passed.

Many thanks for the hard work.

These changes have been spread upstream and are already available in the git and cvs repositories.

Ciao