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

Update defaults in dummy application #4047

Conversation

waiting-for-dev
Copy link
Contributor

Here it's a description of the reason for each change. Keep in mind that
CI test against Rails 5.2 as the minimal version:

  • serve_static_assets: Removed it. It's not available since Rails
    5.0
    .

  • public_file_server.enabled: Set explicitly to true. Even if it was
    already true, a real Rails application will default it to false in
    the production environment unless a env variable
    RAILS_SERVE_STATIC_FILES is
    present
    . This way it's less surprising.

  • whiny_nils: Removed. It's not available since Rails
    4.1
    .

  • action_controller.allow_forgery_protection: Set to true to mimic
    production.

  • action_controller.default_protect_from_forgery: Set to true to mimic
    production.

  • action_controller.perform_caching: Set to true to mimic production.

  • active_support.deprecation: Remove duplicated entry.

  • action_mailer.delivery_job: Removed because load_defaults take care of
    it.

  • storage_path: Removed. It wasn't a Rails setting, but used as a
    convenience. It was confusing.

  • action_controller.include_all_helpers: Removed as it's already the
    default.

  • log_level: Added. It already defaulted to :debug, but a real Rails app
    will default to :info in production. To make things less surprising
    let's be explicit.

  • action_mailer.perform_caching: Added to mimic production.

  • i18n.fallbacks: Added to mimic production.

  • active_record.dump_schema_after_migration: Added to mimic production and
    for better performance.

  • assets: No need to check whether it's a method on config.

See #4035

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@waiting-for-dev
Copy link
Contributor Author

Hmm. Some tests failing on CI but not on my local machine. I'll try to figure it out.

@waiting-for-dev
Copy link
Contributor Author

I think it'll be green once #4048 is merged

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/review_dummy_app_settings branch 2 times, most recently from 077729e to f523921 Compare May 7, 2021 08:32
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks, @waiting-for-dev.

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/review_dummy_app_settings branch from 6208ac4 to 556596a Compare June 1, 2021 03:28
@kennyadsl
Copy link
Member

@waiting-for-dev did we remove the grouping suggested by Jared with the last commit?

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/review_dummy_app_settings branch from 556596a to ca0603d Compare June 1, 2021 09:06
@waiting-for-dev
Copy link
Contributor Author

@waiting-for-dev did we remove the grouping suggested by Jared with the last commit?

Thanks for catching it @kennyadsl . As I directly committed the suggestion but didn't update my local repo, I override when forced the push 🙈 Sorry for that 😞

Just fixed it.

@kennyadsl
Copy link
Member

No problem, we are here to review!

Here it's a description of the reason for each change. Keep in mind that
CI test against Rails 5.2 as the minimal version:

- serve_static_assets: Removed it. It's [not available since Rails
5.0](https://guides.rubyonrails.org/5_0_release_notes.html#railties-removals).

- public_file_server.enabled: Set explicitly to `true`. Even if it was
already `true`, a [real Rails application will default it to `false` in
the production environment unless a env variable
`RAILS_SERVE_STATIC_FILES` is
present](https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt#L27). This way it's less surprising.

- whiny_nils: Removed. It's [not available since Rails
4.1](https://guides.rubyonrails.org/4_1_release_notes.html#railties-removals).

- action_controller.allow_forgery_protection: Set to true to mimic
  production.

- action_controller.default_protect_from_forgery: Set to true to mimic
  production.

- action_controller.perform_caching: Set to true to mimic production.

- active_support.deprecation: Remove duplicated entry.

- action_mailer.delivery_job: Removed because `load_defaults` take care of
it.

- storage_path: Removed. It wasn't a Rails setting, but used as a
convenience. It was confusing.

- action_controller.include_all_helpers: Removed as it's already the
default.

- log_level: Added. It already defaulted to `:debug`, but a real Rails app
will default to `:info` in production. To make things less surprising
let's be explicit.

- action_mailer.perform_caching: Added to mimic production.

- i18n.fallbacks: Added to mimic production.

- active_record.dump_schema_after_migration: Added to mimic production and
for better performance.

- assets: There's no need to check whether it's a method on `config`.

See solidusio#4035
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/review_dummy_app_settings branch from ca0603d to 93529a6 Compare June 2, 2021 10:03
@kennyadsl kennyadsl merged commit bb385e6 into solidusio:master Jun 9, 2021
@kennyadsl kennyadsl deleted the waiting-for-dev/review_dummy_app_settings branch June 9, 2021 12:25
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