Skip to content

Comments

Having a @request variable defined in user's controller could be problematic#144

Merged
markets merged 3 commits intomasterfrom
fix_access_to_request_object
Jan 30, 2025
Merged

Having a @request variable defined in user's controller could be problematic#144
markets merged 3 commits intomasterfrom
fix_access_to_request_object

Conversation

@markets
Copy link
Owner

@markets markets commented Jan 22, 2025

Closes #105

More details 👉🏼 #105 (comment). It seems we probably don't need to memoize this variable ... so instead of renaming it to something like @_current_request (which will also fix the issue), I think we can just ✂️

@markets
Copy link
Owner Author

markets commented Jan 22, 2025

@cryptogopher Could you please test this branch in your app?

NOTE Those CI failures seem totally unrelated ("uninitialized constant ActiveSupport::LoggerThreadSafeLevel::Logger"), specs are passing fine ✅ for me locally. They seem to fail only in Rails < v7.1.

NOTE 2 CI failures seem to be related with this: rails/rails#54264

@cryptogopher
Copy link

All is good, thank you

@markets
Copy link
Owner Author

markets commented Jan 22, 2025

Cool 👌🏼 thanks for checking!

I'll see how can I resolve those CI failures and release a new version soon (probably this week).

@cryptogopher
Copy link

cryptogopher commented Jan 22, 2025

Regarding CI failures: if I'm not mistaken, there is a problem with gem 'concurrent-ruby' 1.3.5. You can try to pin it to 1.3.4 and see if it helps.

https://stackoverflow.com/a/79360861/7886032

@markets
Copy link
Owner Author

markets commented Jan 22, 2025

That change must have broken many CIs!

I think it should be fixed upstream, it doesn't make a lot of sense 😅 to force a whole community of library maintainers to pin to specific versions and make changes to fix some incompatibilities with gems that we don't even use.

@markets markets marked this pull request as ready for review January 23, 2025 19:54
@markets
Copy link
Owner Author

markets commented Jan 23, 2025

@cryptogopher I just pushed a workaround for the concurrent-ruby thing (Rails 7.0 ✅), but now I remember why I added this @request thing here. In Rails 6, there is no access to this request object at this point, at least when running the tests (now I have doubts if this is something that happens only when running in "test" mode or it will crash also when using the real app).

@markets
Copy link
Owner Author

markets commented Jan 27, 2025

UPDATE It seems this code (access to the @request ivar) is only necessary inside tests, in a regular Rails 6 app you can use the request object with no problems, so I'll refactor this part (probably by mocking the request object) and we'll be able to ship this fix.

@markets
Copy link
Owner Author

markets commented Jan 28, 2025

@cryptogopher Seems that 54c9656 fixed the build, I'd say this is ready ✅

@markets markets merged commit 97640fd into master Jan 30, 2025
32 checks passed
@markets markets deleted the fix_access_to_request_object branch January 30, 2025 10:11
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.

Undefined method remote_ip

2 participants