Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-14679 META: DB layer 2.0
  3. MDL-14877

prefix check shouldn't be perfomed by moodle_database classes

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Database SQL/XMLDB
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Right now prefix rules are executed by the xxx_yyy_moodle_database constructor.

      That causes 2 problems:

      1) force ALL db to fulfil the rules (not only Moodle DB, but auth/enrol....)
      2) print_error() output is 100% broken, because a lot of stuff hasn't been defined (lang, themes....)

      IMO those checks should continue where they are now (admin/index.php), simply preventing any Moodle installation, but not making the dml/ddl stuff dependent of that.

      Ciao

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            skodak Petr Skoda added a comment -

            1/ oh - the only problem might be reserved words in temp and externale talbes - my +1 to revert this
            2/ sure

            Show
            skodak Petr Skoda added a comment - 1/ oh - the only problem might be reserved words in temp and externale talbes - my +1 to revert this 2/ sure
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Fixed. Working. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Fixed. Working. Ciao
            Hide
            skodak Petr Skoda added a comment - - edited

            Thinking a bit more about this I would really like to get all those get_dbfamily() out - this looks like a step backwards

            We could add a new moodle_database method or a new constructor parameter $testprefix=true, then we could choose to test it (or not) and the admin/index.php and CLI installer would not be polluted with dbfamily specific stuff again.

            Show
            skodak Petr Skoda added a comment - - edited Thinking a bit more about this I would really like to get all those get_dbfamily() out - this looks like a step backwards We could add a new moodle_database method or a new constructor parameter $testprefix=true, then we could choose to test it (or not) and the admin/index.php and CLI installer would not be polluted with dbfamily specific stuff again.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Well,

            we were discussing time ago about get_family() being public or no. And finally agree about to leave them public because could be needed in some places.

            IMO this is exactly the type of place where it can be used. They are only two simple and stopper checks in the right place.

            Sincerely, I think it's a lot more polluting to add new parameters to constructor and so. I think that moodle_databases shouldn't perform those tests conditionally at all. As a last chance... I see them in another public method:

            $DB->check_prefix_rules()

            or so, but leaving, in any case constructors free for that sort of conditional stuff.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Well, we were discussing time ago about get_family() being public or no. And finally agree about to leave them public because could be needed in some places. IMO this is exactly the type of place where it can be used. They are only two simple and stopper checks in the right place. Sincerely, I think it's a lot more polluting to add new parameters to constructor and so. I think that moodle_databases shouldn't perform those tests conditionally at all. As a last chance... I see them in another public method: $DB->check_prefix_rules() or so, but leaving, in any case constructors free for that sort of conditional stuff. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Not the best solution IMO (the external attribute on constructor) but works.

            Committed some days ago in MDL-14864. Resolving as fixed.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Not the best solution IMO (the external attribute on constructor) but works. Committed some days ago in MDL-14864 . Resolving as fixed.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  24/Nov/10