Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31689

fix moodle_database::execute() comment or change database_manager::execute_sql to public function

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Database SQL/XMLDB
    • Labels:
      None

      Description

      moodle_database.php execute() function has this comment:
      Do NOT use this to make changes in db structure, use database_manager::execute_sql() instead!

      but - execute_sql() is a protected function so we can't use it.

      We have some custom code that uses custom views/functions and it's a lot easier to create these on the db importing a pg_dump file. - they don't use moodle prefix etc. $DB->execute does the prefix/params stuff and it would be nice to be able to avoid that if poss.

      It would be nice to be able to just use $dbman->execute_sql to do this and according to the comment in moodle_database.php it looks like someone thought it should be supported at some point.

      Can we please either - change the function to a public one - or remove the invalid comments in moodle_database.php that tell us to use it. (my preference is to change to public!)

        Gliffy Diagrams

          Activity

          Hide
          skodak Petr Skoda added a comment -

          1. the comment in moodle_database::execute() was supposed to say that you should always use cross-db methods from database manager - I am fixing it right now

          2. the database_manager is supposed to do only standardised changes in moodle database, my -2 for making the execute_sql() there public because it would encourage bad coding style

          3. you must use bound parameters always - no exceptions, sorry

          4. you may even use the old style prefix concat instead of

          {table}

          , I can not see any problems in $DB->execute() that would be caused by prefixes

          I am going to have similar problem in MDL-27982 because I am going to hardcode mysql upgrade hack in lib/db/upgradelib.php that removes unsigned integers, I would reopen this if I find any problems there.

          Thanks a lot for the report, please comment here if I overlooked something, I am going to test it this week...

          Show
          skodak Petr Skoda added a comment - 1. the comment in moodle_database::execute() was supposed to say that you should always use cross-db methods from database manager - I am fixing it right now 2. the database_manager is supposed to do only standardised changes in moodle database, my -2 for making the execute_sql() there public because it would encourage bad coding style 3. you must use bound parameters always - no exceptions, sorry 4. you may even use the old style prefix concat instead of {table} , I can not see any problems in $DB->execute() that would be caused by prefixes I am going to have similar problem in MDL-27982 because I am going to hardcode mysql upgrade hack in lib/db/upgradelib.php that removes unsigned integers, I would reopen this if I find any problems there. Thanks a lot for the report, please comment here if I overlooked something, I am going to test it this week...
          Hide
          skodak Petr Skoda added a comment -

          Could you please post here an example of your SQL?

          Show
          skodak Petr Skoda added a comment - Could you please post here an example of your SQL?
          Hide
          skodak Petr Skoda added a comment -

          to integrators: I guess Eloy should review this

          Show
          skodak Petr Skoda added a comment - to integrators: I guess Eloy should review this
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          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

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - 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
          Hide
          skodak Petr Skoda added a comment -

          rebased

          Show
          skodak Petr Skoda added a comment - rebased
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hehe spotted comments, reassigning Eloy as integrator

          Show
          samhemelryk Sam Hemelryk added a comment - Hehe spotted comments, reassigning Eloy as integrator
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrating the docs changes and backporting them as much as possible.

          So Dan, then, the conclusion is that, for non moodle db objects, it's correct (the only way, in fact) to use $DB->execute(). You always can pass empty params, hardcode them, ignore prefixes there. It's your code.

          BTW.. perhaps we should analyze the possibility of adding support to (non-materialized) views from xmldb/db manager. I think it's more or less a cross-db feature.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrating the docs changes and backporting them as much as possible. So Dan, then, the conclusion is that, for non moodle db objects, it's correct (the only way, in fact) to use $DB->execute(). You always can pass empty params, hardcode them, ignore prefixes there. It's your code. BTW.. perhaps we should analyze the possibility of adding support to (non-materialized) views from xmldb/db manager. I think it's more or less a cross-db feature. Ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated by cherry pick and backported to 21 and 22 stables, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated by cherry pick and backported to 21 and 22 stables, thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Confirming it's only comments change, so passing.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Confirming it's only comments change, so passing.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

          icao_reverse('arreis olik rebemevon afla letoh ognat');

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Mar/12