Fix ActiveRecord::Migrator#needs_migration? inconsistency (fix #10856) #13589

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@907th
Contributor

907th commented Jan 4, 2014

Issue (see #10856 too):
Consider the situation when there are 3 migrations:
* 1st is up,
* 2nd is down,
* 3rd is up.
In such case ActiveRecord::Migrator.needs_migration? returns false,
but it should return true.

Consequences:
Pending migrations check in development environment will not work
(see config.active_record.migration_error option).

Solution:
Consider all migrations while checking for pending ones
(not only the latest migration).

Tests were added for the issue and ActiveRecord::Migration::CheckPending
middleware.

fix ActiveRecord::Migrator#needs_migration? inconsistency (fix #10856)
Issue (see #10856 too):
  Consider the situation when there are 3 migrations:
    * 1st is up,
    * 2nd is down,
    * 3rd is up.
  In such case `ActiveRecord::Migrator.needs_migration?` returns `false`,
  but it should return `true`.

Consequences:
  Pending migrations check in development environment will not work
  (see `config.active_record.migration_error` option).

Solution:
  Consider all migrations while checking for pending ones
  (not only the latest migration).

Tests were added for the issue and `ActiveRecord::Migration::CheckPending`
middleware.
@907th

This comment has been minimized.

Show comment Hide comment
@907th

907th Jan 5, 2014

Contributor

@rafaelfranca Could you please restart the stalled build on Travis? Thanks!

Contributor

907th commented Jan 5, 2014

@rafaelfranca Could you please restart the stalled build on Travis? Thanks!

@907th

This comment has been minimized.

Show comment Hide comment
@907th

907th Jan 21, 2014

Contributor

@tenderlove Please, take a look at it. It seems like you were the last person who fixed ActiveRecord::Migration::CheckPending and added mtime checks.

Contributor

907th commented Jan 21, 2014

@tenderlove Please, take a look at it. It seems like you were the last person who fixed ActiveRecord::Migration::CheckPending and added mtime checks.

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 3, 2014

Member

The code in question needs_migration? has changed considerably. The current code should already honor the case described in this PR.

@907th thank you for your work 💛

Member

senny commented Dec 3, 2014

The code in question needs_migration? has changed considerably. The current code should already honor the case described in this PR.

@907th thank you for your work 💛

@senny senny closed this Dec 3, 2014

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