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

Two small (but important) fixes #1525

Merged
merged 5 commits into from
Sep 23, 2022
Merged

Two small (but important) fixes #1525

merged 5 commits into from
Sep 23, 2022

Conversation

nagmat84
Copy link
Collaborator

This PR fixes to bugs I came across while I had been working on #1522.

  1. A newer version of some package does not call the method User::name() on the user model anymore, but assumes, that name is an attribute and calls User::$name. Hence, we need the accessor function User::getNameAttribute() to please Laravel.
  2. The migration 2019_10_03_214750_frame_refresh_in_sec does not use DB methods only, but depends on particular productive code which breaks when this productive code changes.

Appeal

We should really stop to use production code in our migrations. That is a bad thing to do. The migrations will exist "forever" in some sense and even very old migrations must still be runnable in the future. If they use production code and make certain assumptions about that, this means we can never change that code without breaking those migrations. And nobody feels like fixing ancient migrations.

This particular issue took me two hours to debug in the step debugger. I encountered really weird and seemingly unrelated error messages like ConfigurationMissingException: No such key map_provider for a lot of seemingly random configuration settings. I then noticed, that I only had about 60 configuration items in my test DB instead of the usual 90. "Something" was deleting about 30 configuration items. Then I found out that it happened during the Unit Tests when the migration was tested and finally I found the wrongdoer in that particular migration.

@nagmat84 nagmat84 requested a review from a team September 22, 2022 17:28
@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Merging #1525 (8eb288c) into master (5884d4a) will decrease coverage by 0.73%.
The diff coverage is 66.66%.

Additional details and impacted files

Copy link
Contributor

@kamil4 kamil4 left a comment

Choose a reason for hiding this comment

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

I believe I wrote that migration in the first place. In my defense, that was in 2019, and I simply didn't realize the consequences at the time...

@ildyria ildyria merged commit c0a90b7 into master Sep 23, 2022
@ildyria ildyria deleted the two_fixes branch September 23, 2022 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants