Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove null=true from database schema, fixing #1016 #1018

Closed
wants to merge 1 commit into from

Conversation

takkaria
Copy link
Contributor

@takkaria takkaria commented Sep 15, 2018

As a stopgap, this fixes #1016, which prevents a new instance of EmonCMS on a blank database being created. It is caused by a bug in the database schema code but this is a good stopgap fix.

Fields can be Null by default so there would be no point
in specifying it even if the database schema code didn't
treat it wrongly.
@TrystanLea
Copy link
Member

Thanks @takkaria there seems to be an ongoing issue here that Im not getting to the bottom of.. as we've been adding and removing this multiple times, here's my last commit where I added it in to fix the same issue... 2c913bf

Seems to be caused by differences in the installation environment somehow?

@takkaria
Copy link
Contributor Author

takkaria commented Sep 19, 2018

@TrystanLea so the problem is that the code in dbschemasetup.php is really inconsistent. I tried to explain what was going on in pull request #930 and also in a comment on issue #791.

I imagine the reason this fix works for a new person but doesn't work for you is because you're not testing with a clean database which means that when you run the database schema update check, you are running a different code path to someone who is setting up from scratch (table creation vs table alteration).

I have just spent 20 minutes trying to write an explaination the bug and failing, because the code in the schema setup file is very difficult to follow. Which is kind of the problem! The logic for the NOT NULL constraints is different in the three different cases database definition alterations are made:

  1. creating a table from scratch
  2. creating a column on an existing table
  3. altering the definition of an existing column

I already wrote an explanation of why it fails in the first case. As far as I can tell, in the second case, in order to change a column to have a NOT NULL constraint, you'd have to have the 'Null' value in the database schema definition set to "NO". And in the third case, to make it produce NOT NULL constraint, you'd have to have it set the schema definition 'Null' key to anything other than 'true'. So it's quite inconsistent.

It's further complicated by the fact that you have to respecify the whole column spec when you run ALTER TABLE, not just the alterations. At the moment cases 2 and 3 only attempt to respecify the bits of the column spec that have changed, which means that different EmonCMS instances with the same verison of EmonCMS are likely to have different database schemas depending on how many upgrades they've had.

I'd suggest taking a look at the pull request for #930, because I fixed all the issues I could find with this code there! And it makes my head hurt a bit trying to explain it! Hope this all makes sense.

@TrystanLea
Copy link
Member

Thankyou @takkaria I will prioritise looking at #930 thankyou for the reminder and for highlighting that there are inconsistencies that this sorts.

@TrystanLea
Copy link
Member

I've merged #930, can this pull request now be closed?

@takkaria
Copy link
Contributor Author

Yup!

@takkaria takkaria closed this Sep 23, 2018
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.

MySQL database setup for user registration.
2 participants