The database audit tool output is meant to be a guideline. It should not be treated as a database repair tool. That's why I included the original warning "WARNING!!! These commands are only suggestions". For instance, if you had a plugin that added a table, the database audit tool would tell you to drop it. And then your Koha plugin is broken. Not a great outcome.
When I originally wrote the database audit, I thought about outputting the differences differently, but I thought SQL would be the easiest way for people to understand, and it was a simple method provided by SQL::Translator::Diff. Perhaps I should've known that people might not heed the warning.
Note that the impetus for raising this bug was someone on the koha-devel listserv saying they saw the "Warning! These commands are only suggestions" but then they saw "Run the following SQL to fix the database", so they ran the DB audit SQL as is.
Created attachment 184065 [details] [review] Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting This change removes the incorrect instruction from about.pl: "Run the following SQL to fix the database" and fixes the HTML formatting so that the line "-- Convert schema '' to '':;" is displayed correctly at the start of the line. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/about.pl?tab=database 2. Note that "Run the following SQL to fix the database" no longer appears and "--Convert schema '' to '':;" starts at the beginning of the line instead of with leading whitespaces.
I just ran into a fun issue. When you add a FOREIGN KEY in MySQL/MariaDB, it will silently add an index. So if you don't include an index explicitly in kohastructure.sql, the DB audit will mistakenly tell you to drop an index that should not be dropped.
Created attachment 184094 [details] [review] Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting This change removes the incorrect instruction from about.pl: "Run the following SQL to fix the database" and fixes the HTML formatting so that the line "-- Convert schema '' to '':;" is displayed correctly at the start of the line. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/about.pl?tab=database 2. Note that "Run the following SQL to fix the database" no longer appears and "--Convert schema '' to '':;" starts at the beginning of the line instead of with leading whitespaces. Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Created attachment 184095 [details] [review] Bug 40370: (QA follow-up) Change raw filter to html The diff doesn't contain intentional HTML formatting, and pre tags do not prevent un-escaped HTML from being interpreted, so we should not use the raw filter here Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
This is a clearly reasonable string patch; moving straight to PQA. ...with a follow-up to change the $raw filter to html, as the diff doesn't contain intentional HTML formatting and it's recommended to escape HTML characters inside of <pre> tags (https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/pre#escaping_ambiguous_characters). I hope this isn't an overstep, and I'm happy to move the patch to a separate bug if it is! But it didn't feel like a good use of anyone's time to have two separate bugs to make two string edits to the same line. :)
Nice work everyone! Pushed to main for 25.11
(In reply to Emily Lamancusa (emlam) from comment #7) > ...with a follow-up to change the $raw filter to html, as the diff doesn't > contain intentional HTML formatting and it's recommended to escape HTML > characters inside of <pre> tags > (https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/ > pre#escaping_ambiguous_characters). I hope this isn't an overstep, and I'm > happy to move the patch to a separate bug if it is! But it didn't feel like > a good use of anyone's time to have two separate bugs to make two string > edits to the same line. :) Thanks, Emily. Not an overstep at all. It was a great choice. The thought crossed my mind yesterday, but then I got side-tracked with other things. (Funny enough working on this bug showed me some DB issues with a different bug heh.)