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 issue with using draper outside of controller/view context #927

Merged

Conversation

timdiggins
Copy link
Contributor

@timdiggins timdiggins commented Aug 1, 2023

Description

Replace ActionController::TestRequest with ActionDispatch::TestRequest

because

  1. soft deprecated
  2. will cause very hard to predict/test failures with ActiveJob/Sidekiq/Rake tasks. see issue with using draper outside of controller/view context within a rake task or (active)job #926

The current PR also drops support for rails < 6.0
I think this is a good idea (especially as the tests that were dropped were written from a Rails 5.0 perspective, so are very outdated, and brittle) but open to discussion around this if there's any engagement on this PR.

Testing

See #926

To-Dos

  • tests: I have not done additional tests to prove the need for the PR as they are hard to implement and harder to maintain and wouldn't catch regressions in any case.
  • documentation: unnecessary I think

References

fixes #926

@tovodeverett
Copy link
Contributor

I have no authority at all with Draper, so I'm sorry that I can't help with getting your PR approved.

I do have one comment - I found the first line of the Description to be rather confusing.

You wrote: "Replace ActionController::TestRequest with ActionController::TestRequest"
I think you may have meant to write, "Replace ActionController::TestRequest with ActionDispatch::TestRequest"

gentoo-repo-qa-bot pushed a commit to gentoo-mirror/graaff that referenced this pull request Jan 2, 2024
@mdudzinski
Copy link

mdudzinski commented Mar 13, 2024

I can confirm this PR fixed the problem (that popped up after upgrading to rails 6.1) in my project.

In my case it was:

NoMethodError: undefined method `parameters' for nil:NilClass

@_params ||= Parameters.new(request.parameters)

~/.gem/ruby/3.1.4/gems/actionpack-6.1.7.7/lib/action_controller/metal/strong_parameters.rb:1187:in `params'

Thank you @timdiggins !

Copy link
Collaborator

@Alexander-Senko Alexander-Senko left a comment

Choose a reason for hiding this comment

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

👍 for dropping support of Rails 5, but could we split it from the fix?

The reason is that support for older Rails & Ruby versions is far more complicated than that of the code changed in this PR. E.g. we still have a CI for Ruby 2.4 that's incompatible with Rails 6.

lib/draper/view_context/build_strategy.rb Show resolved Hide resolved
fixes drapergem#926

however the current PR also drops support for rails < 6.0
open to discussion around this if there's any engagement on this PR.
@timdiggins timdiggins force-pushed the use-other-test-request branch from ca64cc8 to 6e0be67 Compare September 1, 2024 18:32
Copy link
Collaborator

@Alexander-Senko Alexander-Senko left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution 🙏

@Alexander-Senko Alexander-Senko merged commit a93223e into drapergem:master Sep 1, 2024
8 checks passed
@Alexander-Senko Alexander-Senko added this to the 4.0.3 milestone Oct 4, 2024
Alexander-Senko added a commit that referenced this pull request Oct 4, 2024
Added support for latest Ruby (upto 3.4) and Rails (upto 8) versions.

## Fixes

* `CollectionDecorator#respond_to?` for non-ORM collections
  (#920)
* Using Draper outside of controller scope
  (#927)
* Decoration of AR associations
  (#932)

## Performance

* Sped up delegation via `delegate_all`
  (#911)
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.

issue with using draper outside of controller/view context within a rake task or (active)job
4 participants