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

Fix deprecation warnings for secrets:edit/show #49792

Conversation

stevegeek
Copy link
Contributor

Motivation / Background

This Pull Request has been created because there is a bug in the deprecation notices on the tasks secrets:edit and secrets:show which means they can raise undefined method 'squish'

Reproduction

Create a new rails 7.1.1 app using rails new.

cd into the new app and run bin/rails secrets:edit or bin/rails secrets:show.

For example:

$ bin/rails secrets:show
...gems/3.2.0/gems/railties-7.1.1/lib/rails/commands/secrets/secrets_command.rb:37:in `show': undefined method `squish' for "`bin/rails secrets:show` is deprecated in favor of credentials and will be removed in Rails 7.2.\nRun `bin/rails credentials:help` for more information.\n":String (NoMethodError)

        Rails.deprecator.warn(<<~MSG.squish)
                                    ^^^^^^^
	from ...gems/3.2.0/gems/thor-1.3.0/lib/thor/command.rb:28:in `run'
        ...

Detail

The warning string is #squished however active_support/core_ext/string/filters is not being required.

The tests are passing because the dummy test application does a require 'rails/all' on boot which requires the String extensions. https://github.com/rails/rails/blob/main/railties/test/isolation/abstract_unit.rb#L605

This is not necessarily the case for other applications, eg ones freshly created with rails new so in this case an error is raised.

The fix is to just require the necessary String extensions.

I am not sure if there is anything that could be done to the tests to have caught this.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.

The warning string is `#squish`ed however `core_ext/string/filters` was
not being required. The tests were passing because the dummy test
application does a `require 'rails/all'` on boot. This is not necessarily
the case for other applications, eg ones freshly created with `rails new`.
@rails-bot rails-bot bot added the railties label Oct 26, 2023
@akhilgkrishnan akhilgkrishnan added this to the 7.1.2 milestone Oct 26, 2023
Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

Thanks @stevegeek 💯 for the PR.

We need to backport this to 7-1-stable.

@jonathanhefner jonathanhefner merged commit 35aa5e3 into rails:main Oct 26, 2023
4 checks passed
@jonathanhefner
Copy link
Member

jonathanhefner commented Oct 26, 2023

Thank you, @stevegeek! ✔️

Backported to 7-1-stable in 54e2165.

jonathanhefner added a commit that referenced this pull request Oct 26, 2023
…ecrets_show_task

Fix deprecation warnings for `secrets:edit/show`

(cherry picked from commit 35aa5e3)
@stevegeek
Copy link
Contributor Author

Thanks @akhilgkrishnan and @jonathanhefner !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants