Moodle
  1. Moodle
  2. MDL-27767 Installation fails if admin->id is not 2
  3. MDL-28595

general exception message error about creating guest or admin users in db/install.php has continue button but continues without trying to reinstall tables.

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 2.1.2
    • Component/s: Installation
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1/ introduce some fatal error in installer by modifying code or install.xml
      2/ execute the install and verify there is not continue button
      3/ verify the new error string

      Show
      1/ introduce some fatal error in installer by modifying code or install.xml 2/ execute the install and verify there is not continue button 3/ verify the new error string
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w36_MDL-28595_m22_installcontinue
    • Rank:
      17472

      Description

      When the parent bug's error occurs, the continue button doesn't really allow continuation.

      The tables or more specifically the data needs to be emptied and re-created. We cannot consider that core_tables_exists() is right can we?

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Sorry. I thought this was a new issue.

          Show
          Michael de Raadt added a comment - Sorry. I thought this was a new issue.
          Hide
          Aparup Banerjee added a comment -

          i've made it add on to the error message a bit about a warning mentioning lacking rollback support. if thats ok i'll dupe to other later branches.

          Show
          Aparup Banerjee added a comment - i've made it add on to the error message a bit about a warning mentioning lacking rollback support. if thats ok i'll dupe to other later branches.
          Hide
          Sam Hemelryk added a comment -

          Hi Apu,

          Just had a quick look at this, 2 things:

          1. Surely lib/setuplib.php should be if (!$DB->transactions_supported()) {
          2. The string should be checked by Helen probably, I think it needs to be clearer.

          In general in regards to this patch does it really help this problem?
          That message gets shown for all exceptions if someone is running with a DB that doesn't support transactions, and to be truthful reading it I feel it is exaggerating the problem and isn't accurate to all exceptions. Especially if its just a missing required_param or an invalid id passed via URL.
          Personally it gets my -1.
          I'd rather see the message improved for exceptions that may lead to data corruption through the lack of transactional support, or just a new type of exception_with_possible_corruption.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Apu, Just had a quick look at this, 2 things: Surely lib/setuplib.php should be if ( ! $DB->transactions_supported()) { The string should be checked by Helen probably, I think it needs to be clearer. In general in regards to this patch does it really help this problem? That message gets shown for all exceptions if someone is running with a DB that doesn't support transactions, and to be truthful reading it I feel it is exaggerating the problem and isn't accurate to all exceptions. Especially if its just a missing required_param or an invalid id passed via URL. Personally it gets my -1. I'd rather see the message improved for exceptions that may lead to data corruption through the lack of transactional support, or just a new type of exception_with_possible_corruption. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Thanks Sam, I agree it was a really quick ugly patch. I'll look into the new exception type for more accurate error reports.

          Show
          Aparup Banerjee added a comment - Thanks Sam, I agree it was a really quick ugly patch. I'll look into the new exception type for more accurate error reports.
          Hide
          Aparup Banerjee added a comment -

          Eloy, could you please review these changes.

          Show
          Aparup Banerjee added a comment - Eloy, could you please review these changes.
          Hide
          Andrew Davis added a comment -

          "//@todo set this privately instead within rollback attempts."

          Is that todo significant? Its ok if its meant to be there just so long as you're aware of it

          Should that corruption flag be getting persisted to the db? Again, maybe its correct, Im not familiar with this part of Moodle.

          Do you need a particular database to test this? If so add that to the testing instructions.

          Show
          Andrew Davis added a comment - "//@todo set this privately instead within rollback attempts." Is that todo significant? Its ok if its meant to be there just so long as you're aware of it Should that corruption flag be getting persisted to the db? Again, maybe its correct, Im not familiar with this part of Moodle. Do you need a particular database to test this? If so add that to the testing instructions.
          Hide
          Aparup Banerjee added a comment - - edited

          yea that todo is about the way the roll back call is simply an attempt to roll back alone, it doesn't really flag anything by itself if it can't rollback )it doesn't do any checks too), hence this tdo to wrap all that up and wrap it up within the DB layer.

          hm persisting the flag. i'm not sure. the only way to do that would be through files? persisting through the db is a bit awkward seeing that we're trying to flag a that the db itself is possibily corrupt.

          Yes its for the mysql setup reported in the parent issue. i'll set the environment as well. (the db type is mentioned in the test)

          Show
          Aparup Banerjee added a comment - - edited yea that todo is about the way the roll back call is simply an attempt to roll back alone, it doesn't really flag anything by itself if it can't rollback )it doesn't do any checks too), hence this tdo to wrap all that up and wrap it up within the DB layer. hm persisting the flag. i'm not sure. the only way to do that would be through files? persisting through the db is a bit awkward seeing that we're trying to flag a that the db itself is possibily corrupt. Yes its for the mysql setup reported in the parent issue. i'll set the environment as well. (the db type is mentioned in the test)
          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
          Sam Hemelryk added a comment -

          Stopping review detailed comment coming soon...

          Show
          Sam Hemelryk added a comment - Stopping review detailed comment coming soon...
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Eloy, I'm making you integration reviewer of this issue so that you can take a peak at it.
          Apu, presently this gets my -1, however lets see what Eloy thinks of it.

          My thoughts on this:

          • In general I don't know about these changes. It's a very powerful message to give as part of an exception in the circumstances that there is POSSIBLY something wrong. Perhaps the warning that is provided just needs to be clearer. Alternatively what if during installation, and then on the admin notifications page we show a warning if the site is using a database that doesn't support transactions.
          • As these are API changes to the database libraries I don't know how suitable they are for branches other than master.
          • I think the method names being added to moodle_database could be improved. Perhaps set_possible_corruption_flag() and has_possible_corruption_occured();
          • All todo's in Moodle code should have an MDL issue number (its' somewhere in the coding guidelines)
          • PHPdocs for warn_corruption uses object - should be bool
          • I'm don't setuplib.php:abort_all_db_transactions should be setting the possible corruption flag, certainly if you're using a database engine that doesn't support transactions you are already aware that rolling back queries isn't a possibility and that for any exception there is a chance of corruption.

          Anyway, keen to see what others think.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Eloy, I'm making you integration reviewer of this issue so that you can take a peak at it. Apu, presently this gets my -1, however lets see what Eloy thinks of it. My thoughts on this: In general I don't know about these changes. It's a very powerful message to give as part of an exception in the circumstances that there is POSSIBLY something wrong. Perhaps the warning that is provided just needs to be clearer. Alternatively what if during installation, and then on the admin notifications page we show a warning if the site is using a database that doesn't support transactions. As these are API changes to the database libraries I don't know how suitable they are for branches other than master. I think the method names being added to moodle_database could be improved. Perhaps set_possible_corruption_flag() and has_possible_corruption_occured(); All todo's in Moodle code should have an MDL issue number (its' somewhere in the coding guidelines) PHPdocs for warn_corruption uses object - should be bool I'm don't setuplib.php:abort_all_db_transactions should be setting the possible corruption flag, certainly if you're using a database engine that doesn't support transactions you are already aware that rolling back queries isn't a possibility and that for any exception there is a chance of corruption. Anyway, keen to see what others think. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, but I don't think this is the way for fixing this. Some thoughts:

          1) We do not "require" ACID right now, so simply cannot change any behavior based on it. See http://tracker.moodle.org/browse/MDL-28644?focusedCommentId=122204&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-122204

          2) No way this can land into stable branches at all.

          3) Really I don't like those constraints being checked (the admin/guest user ids) at all, but that's another story.

          4) If the problem is the continue button, why not, instead, prevent it to appear on installation always? Any exception happening there should be final and installation should be restarted from scratch.

          Thoughts? Petr? Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, but I don't think this is the way for fixing this. Some thoughts: 1) We do not "require" ACID right now, so simply cannot change any behavior based on it. See http://tracker.moodle.org/browse/MDL-28644?focusedCommentId=122204&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-122204 2) No way this can land into stable branches at all. 3) Really I don't like those constraints being checked (the admin/guest user ids) at all, but that's another story. 4) If the problem is the continue button, why not, instead, prevent it to appear on installation always? Any exception happening there should be final and installation should be restarted from scratch. Thoughts? Petr? Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm going to reopen this to give the parts some more time to comment, aiming for a solution about:

          1) not show the continue button on installation exception (this).
          2) point to some META/docs about to make ACID required (that will enable us to really rely on transactions and rollbacks). Something for >= 2.3 as minimum (no way for 2.2).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm going to reopen this to give the parts some more time to comment, aiming for a solution about: 1) not show the continue button on installation exception (this). 2) point to some META/docs about to make ACID required (that will enable us to really rely on transactions and rollbacks). Something for >= 2.3 as minimum (no way for 2.2). Ciao
          Hide
          Aparup Banerjee added a comment -

          Thanks for the reviews!

          not showing 'continue' makes sense but once admin tries installation again - how do we do 'installation should be restarted from scratch'?

          The DB structure has already been created up to a point and also Some data (admin,guest,context etc).

          Eloy, seeing that we couldn't roll back (this case), are you suggesting we drop all tables based on some flag in session/db or just always drop our tables when (re)starting installation?

          Show
          Aparup Banerjee added a comment - Thanks for the reviews! not showing 'continue' makes sense but once admin tries installation again - how do we do 'installation should be restarted from scratch'? The DB structure has already been created up to a point and also Some data (admin,guest,context etc). Eloy, seeing that we couldn't roll back (this case), are you suggesting we drop all tables based on some flag in session/db or just always drop our tables when (re)starting installation?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Ah, no.

          I'm suggesting to do nothing. Just throw the exception and done (without continue button).

          If the installation has reached one point where it cannot continue, the same error will be shown over and over and over.

          And the admin installing should drop / empty the DB completely, no way we do that for him/her (can exist other tables...whatever, admin's responsibility, not Moodle deciding/performing any destructive action).

          Finally note that, talking about cross-db support, only DML commands (select/insert/update/delete) are part of one transaction and not the DDL (create/drop/alter) one. So no matter if we make all DBs ACID required... it will apply only for DMLs, never for DDLs, because not all DBs consider DDL being part of transactions.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Ah, no. I'm suggesting to do nothing. Just throw the exception and done (without continue button). If the installation has reached one point where it cannot continue, the same error will be shown over and over and over. And the admin installing should drop / empty the DB completely, no way we do that for him/her (can exist other tables...whatever, admin's responsibility, not Moodle deciding/performing any destructive action). Finally note that, talking about cross-db support, only DML commands (select/insert/update/delete) are part of one transaction and not the DDL (create/drop/alter) one. So no matter if we make all DBs ACID required... it will apply only for DMLs, never for DDLs, because not all DBs consider DDL being part of transactions. Ciao
          Hide
          Aparup Banerjee added a comment -

          Thanks Eloy, glad for your feedback (& my learning ).

          Show
          Aparup Banerjee added a comment - Thanks Eloy, glad for your feedback (& my learning ).
          Hide
          Aparup Banerjee added a comment -

          ok, i've fixed some issues seen from reviews also fixed todo - created MDL-29234.

          Putting this up for integration on master only now. It just displays a warning about not rolling back and theres is no continue button now.

          Show
          Aparup Banerjee added a comment - ok, i've fixed some issues seen from reviews also fixed todo - created MDL-29234 . Putting this up for integration on master only now. It just displays a warning about not rolling back and theres is no continue button now.
          Hide
          Aparup Banerjee added a comment -

          just correcting the link type Michael added

          Show
          Aparup Banerjee added a comment - just correcting the link type Michael added
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Sorry guys, I'm not going to integrate this. The more I think, the less I understand why we need all that "artillery" added to database drivers.

          I only know 2 things:

          • We do not require ACID compliance right now. That's a fact.
          • Any exception @ install should end with FINAL error message, aka, no possible to continue at all. Button is 100% nosense. We can show specialised message (please, delete and restart from scratch) or whatever we want, but continue button...

          And also (but unrelated with the "continue" above:

          • We don't know if the DB supports transactions and, more yet, we don't know if the DB supports DDL in transactions. So the status at the end of the exception handler execution on install is 100% unknown. That re-enforces the idea of not allowing to "continue", because we could/could not be able to continue. Restart is the only way.

          So please, let's talk in Thursday meeting or whatever. But I don't like any code related with transactions / rollback to land for.. nothing?

          Sorry & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Sorry guys, I'm not going to integrate this. The more I think, the less I understand why we need all that "artillery" added to database drivers. I only know 2 things: We do not require ACID compliance right now. That's a fact. Any exception @ install should end with FINAL error message, aka, no possible to continue at all. Button is 100% nosense. We can show specialised message (please, delete and restart from scratch) or whatever we want, but continue button... And also (but unrelated with the "continue" above: We don't know if the DB supports transactions and, more yet, we don't know if the DB supports DDL in transactions. So the status at the end of the exception handler execution on install is 100% unknown. That re-enforces the idea of not allowing to "continue", because we could/could not be able to continue. Restart is the only way. So please, let's talk in Thursday meeting or whatever. But I don't like any code related with transactions / rollback to land for.. nothing? Sorry & ciao
          Hide
          Aparup Banerjee added a comment -

          Eloy,
          the current patch isn't stressing about ACID compliance at all now. Also it does remove the continue button for those 'wrong id detected' failures.

          Are you suggesting that we simply end with FINAL (no continue) error for ALL installation errors instead of just the irrecoverable ones?

          Show
          Aparup Banerjee added a comment - Eloy, the current patch isn't stressing about ACID compliance at all now. Also it does remove the continue button for those 'wrong id detected' failures. Are you suggesting that we simply end with FINAL (no continue) error for ALL installation errors instead of just the irrecoverable ones?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, I'm arguing we are not able to know which installation error is recoverable and which one is not, are we? Do we know always which is the next step to execute?

          IMO (note it is only IMO), ANY installation error should lead to 1) message explaining the cause as verbosely as possible and 2) STOP.

          Not sure if I'm missing something, surely I am. But I cannot find the point about allowing to continue after any error on install. When should we allow that?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I'm arguing we are not able to know which installation error is recoverable and which one is not, are we? Do we know always which is the next step to execute? IMO (note it is only IMO), ANY installation error should lead to 1) message explaining the cause as verbosely as possible and 2) STOP. Not sure if I'm missing something, surely I am. But I cannot find the point about allowing to continue after any error on install. When should we allow that? Ciao
          Hide
          Aparup Banerjee added a comment -

          well 'continue' really does a check to attempt restart of the installation by trying to detect whether core_tables_exist().

          Maybe we can know a little bit about the next step to execute, if we could add on to this check(sum?) to also check for maybe core_data_exist() (system context, guest, admin, roles/perms etc), then perhaps we could resume the installation by attempting to fix the missing core records.

          This could also function as a possible warning to admins - ie: checks that core data are intact.

          that said,maybe thats all much ado about nothing . error and STOP is definitely a safe way but which relies more on the admin for recovery (also our verbose explanation). Where do we go now?

          Show
          Aparup Banerjee added a comment - well 'continue' really does a check to attempt restart of the installation by trying to detect whether core_tables_exist(). Maybe we can know a little bit about the next step to execute, if we could add on to this check(sum?) to also check for maybe core_data_exist() (system context, guest, admin, roles/perms etc), then perhaps we could resume the installation by attempting to fix the missing core records. This could also function as a possible warning to admins - ie: checks that core data are intact. that said,maybe thats all much ado about nothing . error and STOP is definitely a safe way but which relies more on the admin for recovery (also our verbose explanation). Where do we go now?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My +1 goes to simpler patch with verbose messaging + STOP (aka, bloody button out on any installation error).

          Of course, unless somebody can explain any utility for that button on install error (I cannot imagine any).

          With verbose messaging explaining / pointing to actions to perform (basically, drop everything and restart from scratch, or commenting about unsupported configurations, or whatever is necessary).

          Show
          Eloy Lafuente (stronk7) added a comment - My +1 goes to simpler patch with verbose messaging + STOP (aka, bloody button out on any installation error). Of course, unless somebody can explain any utility for that button on install error (I cannot imagine any). With verbose messaging explaining / pointing to actions to perform (basically, drop everything and restart from scratch, or commenting about unsupported configurations, or whatever is necessary).
          Show
          Petr Škoda added a comment - This removes the continue link install: https://github.com/skodak/moodle/tree/wip_MDL-28595_m22_installcontinue https://github.com/skodak/moodle/commit/49660e856bc8a512a33f000687d1698d48dc9cbe
          Hide
          Petr Škoda added a comment -

          I am taking over this issue because I disagree with the proposed changes in the DML layer. I am going to simply remove the continue button from install errors and add an explanation that installer errors are usually not recoverable.

          Show
          Petr Škoda added a comment - I am taking over this issue because I disagree with the proposed changes in the DML layer. I am going to simply remove the continue button from install errors and add an explanation that installer errors are usually not recoverable.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          These changes have been integrated now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, These changes have been integrated now. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          reminds me of a certain blue screen, seems slightly drastic to me but glad its over and done with in the sprint.

          Show
          Aparup Banerjee added a comment - reminds me of a certain blue screen, seems slightly drastic to me but glad its over and done with in the sprint.
          Hide
          Andrew Davis added a comment -

          I deleted the contents of install.xml and received the error you added. Passing.

          Show
          Andrew Davis added a comment - I deleted the contents of install.xml and received the error you added. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: