Moodle
  1. Moodle
  2. MDL-31689

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38268

      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!)

        Activity

        Hide
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda added a comment -

        Could you please post here an example of your SQL?

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

        to integrators: I guess Eloy should review this

        Show
        Petr Škoda added a comment - to integrators: I guess Eloy should review this
        Hide
        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
        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
        Petr Škoda added a comment -

        rebased

        Show
        Petr Škoda added a comment - rebased
        Hide
        Sam Hemelryk added a comment -

        Hehe spotted comments, reassigning Eloy as integrator

        Show
        Sam Hemelryk added a comment - Hehe spotted comments, reassigning Eloy as integrator
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

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

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

        Confirming it's only comments change, so passing.

        Show
        Eloy Lafuente (stronk7) added a comment - Confirming it's only comments change, so passing.
        Hide
        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
        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: