Skip to content

Comments

Allow editing of migration file in "Confirm migration" popup in database table editor.#282

Merged
bennothommo merged 8 commits intorainlab:masterfrom
Air-Petr:i279
Jun 9, 2019
Merged

Allow editing of migration file in "Confirm migration" popup in database table editor.#282
bennothommo merged 8 commits intorainlab:masterfrom
Air-Petr:i279

Conversation

@Air-Petr
Copy link
Contributor

It's about our conversation in i279.

The main things:

  1. I've added the Throwable to handle errors in a migration code.
  2. I've edited JS-scripts. Now after applying the migration a database table view (a backend page) is also updates.

@bennothommo
Copy link
Contributor

@Air-Petr Excellent work! This works really well - well done!

Just a couple of minor issues I found during testing, hopefully they shouldn't take too long to sort out:

  • When the migration table designer is refreshed, after saving the migration, the "Add timestamps" and "Add soft deleting support" buttons both disappear.
  • If the "up" method is not available in the edited migration file, either because it is deleted or renamed, the AJAX request to save the migration fails but no error is passed back to the front-end.
  • You will need to remove the note just above the code editor with the message "The migration code is read-only..."

@bennothommo
Copy link
Contributor

On the second point above, I wonder if it might be better to show 2 code editors, one for the "up" migration and one for the "down" migration, and only showing the code for the actual field/table changes to prevent people from being silly and messing about with the migration methods. Any thoughts on that?

@LukeTowers
Copy link
Contributor

@bennothommo As long as we have proper error handling in place to handle people being silly I'd rather it was just one code editor to empower the developer to do whatever they need / want to do. Sometimes you might want to add other methods. As long as we can gracefully handle people being silly there's no need to restrict the flexibility of the tool.

@Air-Petr
Copy link
Contributor Author

@bennothommo I'll try to solve these issues as soon as possible. Thanks for your review.

On the second point above, I wonder...

This is an interesting thought. But I agree with @LukeTowers: there could be some additional methods in the migration file. Though I can't imagine any situation where other methods are really needed 😅 It would be interesting to hear other opinions.

@LukeTowers
Copy link
Contributor

Other methods would be purely for organizational purposes and mostly useful to do DB seeding, migrations by themselves are usually short enough to not need any other methods.

@bennothommo
Copy link
Contributor

@Air-Petr @LukeTowers My justification for the suggestion was simply that if they want to do some sort of complex migration, they can use the Versions area. I feel this popup migration editor (or editors) is more for adding adjustments to the migration that cannot be done through the database table editor, for example, to add indexes or comments.

@Air-Petr
Copy link
Contributor Author

Air-Petr commented Jun 3, 2019

If the "up" method is not available in the edited...

@bennothommo It's strange, but I can't reproduce this issue. I always got Call to undefined method... in the error popup. I've tested with different php versions, but effect is the same. I have no idea how can it be...

@bennothommo
Copy link
Contributor

@Air-Petr Not sure what was happening for me before, but I am getting the same error as you now as well, so that's fine.

I've tested your latest changes and everything is working as it should, so it's all good to merge. Thanks heaps for your contribution! :)

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