Skip to content

Conversation

@RiedleroD
Copy link
Contributor

column seems to be a reserved keyword in postgresql. really cool that moodle never warned me of this.

anyway, this should fix EDUPL-19 if I'm correct.

this PR also:

  • reverts previous change regarding extra debugging on this DB request (it did its job and will be superseded by feat: Sentry API integration #89 later)
  • reverts workarounds I put in place precisely so that I wouldn't have to rename the field (postgres seems to have "interesting" behaviour around inserting stuff without IDs)

do note that inside the code, the field in the *model* is still called "column" - this is on one hand for brevity, and on the other so I don't have to refactor as much.

`column` seems to be a reserved keyword in postgresql.
really cool that moodle never warned me of this.
@RiedleroD RiedleroD requested a review from mcquenji October 9, 2025 19:03
@RiedleroD RiedleroD self-assigned this Oct 9, 2025
@mcquenji
Copy link
Contributor

mcquenji commented Oct 9, 2025

will the db migration work on prod without bricking everything? (not the kanban data, as there is none but recreating the table)

@RiedleroD
Copy link
Contributor Author

will the db migration work on prod without bricking everything?

I'm literally working off an example moodle provides for that so I better hope this works. It worked on the testserver, anyway.

I will say, the upgrade steps might fail on postgres. If that's the case, the upgrade will just not get applied. I fear that in that scenario we'll have to come up with steps to manually edit the database…

@RiedleroD
Copy link
Contributor Author

RiedleroD commented Oct 9, 2025

TL;DR: should work™ but even if not, worst case they'll just not be able to upgrade and we'll have to edit the DB manually

@mcquenji mcquenji changed the title fix: Renamed column to selectedcolumn in DB fix: rename column to selectedcolumn in DB Oct 9, 2025
@mcquenji mcquenji changed the title fix: rename column to selectedcolumn in DB fix: Rename column to selectedcolumn in DB Oct 9, 2025
@mcquenji mcquenji merged commit abdbf0e into main Oct 9, 2025
5 checks passed
@mcquenji mcquenji deleted the EDUPL-19-fix-kanban-move branch October 9, 2025 19:37
@elist-tgm
Copy link

You could have used quotation around your column names. This ist exactly what "" is for in SQL (remember, its not for String limitation ;-) ).

@RiedleroD
Copy link
Contributor Author

I will note that the moodle-internal function ($DB->insert_record()) crashes completely without this PR, which is the whole reason I originally had to implement the workaround. additionally, quoted column identifiers are not supported in all environments (from my limited research) despite it clearly being in the SQL standard. I guess this is why this feature isn't used inside moodle.

Renaming the column was in my opinion the correct approach, since it will avoid future bugs (since none of the moodle-internal functions seem to quote column names). The original workaround stemmed from laziness and shouldn't have been committed in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants