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

Increased the size of the password request table id #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rcraggs
Copy link
Collaborator

@rcraggs rcraggs commented Feb 13, 2017

Previous the system stopped being able to reset passwords after 255 because of the size of the column. There's no need for the id to be that small.

Previous the system stopped being able to reset passwords after 255 because of the size of the column
Copy link
Collaborator

@mcrauwel mcrauwel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should also add an upgrade/downgrade script?

@rcraggs
Copy link
Collaborator Author

rcraggs commented May 18, 2017

How do you suggest this could be done:

  • as a couple of scripts in the install folder that get run manually
  • introduce a 'proper' database migration system into WebPA (e.g. https://phinx.org/)
  • put something in accounts/reset.php that checks and makes the change if it's not already done.

@mcrauwel
Copy link
Collaborator

In the current codebase, I'd go for the first option...
The second would be the best option in general but I don't think the code is ready to support that.
The third I find very ugly and would also require your application database user to have ALTER privileges (something I try to avoid in general...)

@rcraggs
Copy link
Collaborator Author

rcraggs commented May 23, 2017

I've added migrations (option 2) because it was pretty straightforward to do. Sys admins can upgrade their database by following the instructions at the bottom of install.txt

Developers can add new migrations by following the docs for Phinx.

@rcraggs
Copy link
Collaborator Author

rcraggs commented May 30, 2017

On reflection - I think any change like adding a deployment tool needs to be done in it's own issue, and then the password reset issue can be fixed after that one is there, rather than sneaking it in. I suggest we leave this here, and then return to it once I've had a go at adding a 'proper' update mechanism

@Sephster
Copy link
Collaborator

Sephster commented Dec 6, 2021

@rcraggs I'm just reviewing some PRs as releasing a new version of WebPA. The DB backend has completely changed since this PR was submitted. We are now making heavy use of Doctrine so I think it might be worth using Doctrine migrations to facilitate this change. For this reason, I think this PR should be closed. Hope that is ok but let me know if you have any reservations.

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.

3 participants