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

Better missing/extra migration exception page #3005

Merged
merged 2 commits into from
Aug 11, 2022

Conversation

tadhgboyle
Copy link
Member

@tadhgboyle tadhgboyle commented Aug 8, 2022

Currently when a site is missing migrations, we [I] do three things that are not ideal:

  • Throw an exception
    • This is bad because it requires debugging mode to be enabled, or the user to be admin logged in, which means they often have to check the logs to even see the message
  • Missing migrations are determined by just comparing the size of the core/migrations table and the nl2_phinxlog table
    • This is not great because it does not consider the contents of them, and now that we have a new branch system, the likelihood of migrations being a little more messy in dev is much higher, so we should be stricter
  • It does not say which migrations are missing

Screen Shot 2022-08-08 at 08 46 06

This change checks the file names and compares them to the phinxlog table contents, and prints the issues clearly in plain text so they do not need to check their logs.
The page is in plaintext since other fatal issues (wrong php version, missing vendor dir, etc) are also plaintext.

Finally, this is still much faster than forcing phinx to migrate on every page load:

  • Force migrate ~18ms:
    • Screen Shot 2022-08-08 at 08 52 55
  • This solution ~6ms:
    • Screen Shot 2022-08-08 at 08 53 22

@tadhgboyle tadhgboyle added this to the 2.0.x milestone Aug 8, 2022
@tadhgboyle tadhgboyle merged commit 5d56470 into release/2.0.2 Aug 11, 2022
@tadhgboyle tadhgboyle deleted the feat/better-migration-exception branch August 11, 2022 17:45
@tadhgboyle
Copy link
Member Author

Closes #2968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants