Bug 880203 - Review Request: rubygem-strong_parameters - Permitted and required parameters for Action Pack
Summary: Review Request: rubygem-strong_parameters - Permitted and required parameters...
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ken Dreyer
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-11-26 13:42 UTC by Josef Stribny
Modified: 2016-01-04 05:50 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-09-23 07:13:55 UTC
Type: ---


Attachments (Terms of Use)

Description Josef Stribny 2012-11-26 13:42:40 UTC
Spec URL: http://data-strzibny.rhcloud.com/rubygem-strong_parameters.spec
SRPM URL: http://data-strzibny.rhcloud.com/rubygem-strong_parameters-0.1.5-1.fc17.src.rpm
Description: strong_parameters is an implementation of the slice pattern. The basic idea
is to move mass-assignment protection out of the model and into the controller
where it belongs.
Fedora Account System Username: jstribny
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=4728639

Comment 2 Ken Dreyer 2013-09-10 18:37:28 UTC
Hi Josef, I can take this review.

Would you mind unmarking "tests" as %doc? Also it looks like this package should be updated for the latest Ruby guidelines. For example, you should run "gem unpack" and "gem spec" in %prep, "gem build" in %build, etc.

Comment 3 Ken Dreyer 2013-09-10 18:48:22 UTC
Also, please update to the latest upstream (0.2.1), and update the License field to "MIT".

Comment 4 Josef Stribny 2013-09-11 08:53:00 UTC
Hi Ken,

thanks for taking the review!

> Would you mind unmarking "tests" as %doc?
Sure, this is a mistake.

> you should run "gem unpack" and "gem spec" in %prep, "gem build" in %build, etc.
I will disagree with you here, %gem_install macro is completely sufficient and it's used this way in some gems. I am not patching anything, so those steps are redundant.

But I moved it to %build, where it originally belongs.

> update to the latest upstream
Done.

> and update the License field to "MIT"
Good catch, changed.

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5921913
SRPM: http://kojipkgs.fedoraproject.org//work/tasks/1914/5921914/rubygem-strong_parameters-0.2.1-1.fc19.src.rpm

Comment 5 Ken Dreyer 2013-09-12 03:17:22 UTC
(In reply to Josef Stribny from comment #4)
> > you should run "gem unpack" and "gem spec" in %prep, "gem build" in %build, etc.
> I will disagree with you here, %gem_install macro is completely sufficient
> and it's used this way in some gems. I am not patching anything, so those
> steps are redundant.
> 
> But I moved it to %build, where it originally belongs.

Hi Josef,

Please help me understand this. On the Ruby Packaging Guidelines page, the example is clear about this, and gem2rpm's template also follows the unpack -> spec -> pack pattern. I thought the point of adding this in the Fedora 19 Ruby Packaging guidelines was that we would now be building the gem "from source".

Why not propose a change to the packaging guidelines to fit your style?

Comment 6 Josef Stribny 2013-09-12 07:47:22 UTC
> I thought the point of adding this in the Fedora 19 Ruby Packaging guidelines > was that we would now be building the gem "from source".

Maybe. Maybe it's there to match other guidelines better? The package doesn't come with any compiled source, and `gem install` actually compiles the extension if there is any (gems should come with sources, not compiled code) and before that, it actually also unpacks the .gem file.

> Why not propose a change to the packaging guidelines to fit your style?

I believe this was already discussed before the new guidelines were accepted. Besides the guidelines are a good default, which I don't want to change. I just prefer to simplify things where it's possible.

But for piece of mind, I made the change for you:

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5925972
SRPM: http://kojipkgs.fedoraproject.org//work/tasks/5973/5925973/rubygem-strong_parameters-0.2.1-2.fc19.src.rpm

Comment 7 Ken Dreyer 2013-09-12 23:11:32 UTC
(In reply to Josef Stribny from comment #6)
> But for piece of mind, I made the change for you:

Thank you - that is really great :)

I've completed the review, and there are two remaining issues.

The first is really minor. In %install, please use "cp -pa" rather than "cp -a" in order to preserve the file timestamps.

The second is more complex. This gem is not compatible with Rails 4. If I remove the "< 4" version restrictions in the .spec file, and try to build it for Rawhide, the test suite fails:

Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.UAJLf3
+ umask 022
+ cd /builddir/build/BUILD
~/build/BUILD/strong_parameters-0.2.1/usr/share/gems/gems/strong_parameters-0.2.
1 ~/build/BUILD/strong_parameters-0.2.1
+ cd strong_parameters-0.2.1
+ pushd ./usr/share/gems/gems/strong_parameters-0.2.1
+ testrb -Ilib test/action_controller_required_params_test.rb test/action_contro
ller_tainted_params_test.rb test/active_model_mass_assignment_taint_protection_t
est.rb test/controller_generator_test.rb test/log_on_unpermitted_params_test.rb 
test/multi_parameter_attributes_test.rb test/parameters_permit_test.rb test/para
meters_require_test.rb test/parameters_taint_test.rb test/raise_on_unpermitted_p
arams_test.rb

/builddir/build/BUILD/strong_parameters-0.2.1/usr/share/gems/gems/strong_paramet
ers-0.2.1/lib/action_controller/parameters.rb:11:in `<module:ActionController>':
 superclass mismatch for class ParameterMissing (TypeError)


This error led me to:

https://github.com/rails/strong_parameters/issues/45

What does that mean for this package? Is this functionality already in Rails 4?

Comment 8 Vít Ondruch 2013-09-17 11:18:33 UTC
(In reply to Ken Dreyer from comment #7)
> This error led me to:
> 
> https://github.com/rails/strong_parameters/issues/45
> 
> What does that mean for this package? Is this functionality already in Rails
> 4?

This gem is just for Rails 3:

https://github.com/rails/strong_parameters#compatibility

Integrated somehow into Rails 4 since last September:

https://github.com/rails/rails/commit/885005461b3cc0d073ec08495dc3bf06d0bebf2a

The independent strong_parameters gem was probably required by RoR at some point, but I can't find where/how. We should probably close this review.

Comment 9 Josef Stribny 2013-09-23 07:13:55 UTC
> The independent strong_parameters gem was probably required by RoR at some 
> point, but I can't find where/how. We should probably close this review.

True, closing.


Note You need to log in before you can comment on or make changes to this bug.