remove deprecated proprety namespace collisionslcg/remove-deprecated-namespace-collisions

require writing `new_resource.my_property` instead of just
`my_property` in provider code.

a surprise in this code was that methods directly on the
resource are all injected into the provider, and we never
deprecated that, because we didn't know about it.

that makes `action_class {}` helpers unnecessary, but causes
additional namespace pollution, but at least the methods declared
in the resource will lose to DSL methods (i.e. `def execute`
will not work).

but that also means that resources which are injected into the DSL
which overwrite resource methods in other cookbooks may break
their use in that cookbooks providers/action_classes.

we should probably deprecate that as well, but we didn't and
people probably found this behavior and used it.

i cleaned it up a bit and removed the methods on Chef::Resource
directly which polluted the provider class with all kinds of
bizzare crap methods (not_if and only_if were getting injected
into the provider, etc).

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 6 insertions, 130 deletions

diff --git a/lib/chef/provider.rb b/lib/chef/provider.rb
index 7cb2301772..6971465b0a 100644
--- a/lib/chef/provider.rb
+++ b/lib/chef/provider.rb
@@ -342,45 +342,16 @@ class Chef
           extend Forwardable
           define_singleton_method(:to_s) { "forwarder module for #{provider_class}" }
           define_singleton_method(:inspect) { to_s }
-          # Add a delegator for each explicit property that will get the *current* value
-          # of the property by default instead of the *actual* value.
-          resource.class.properties.each_key do |name|
-            class_eval(<<-EOM, __FILE__, __LINE__)
-              def #{name}(*args, &block)
-                # If no arguments were passed, we process "get" by defaulting
-                # the value to current_resource, not new_resource. This helps
-                # avoid issues where resources accidentally overwrite perfectly
-                # valid stuff with default values.
-                #
-                # This magic is to make this kind of thing easy:
-                #
-                # FileUtils.chown new_resource.mode.nil? ? current_resource.mode : new_resource.mode, new_resource.path
-                #
-                # We do this in the file provider where we need to construct a new filesystem object and
-                # when the new_resource is nil/default that means "preserve the current stuff" and does not
-                # mean to ignore it which will wind up defaulting to changing the file to have a "root"
-                # ownership if anything else changes.  Its kind of overly clever and magical, and most likely
-                # gets the use case wrong where someone has a property that they really mean to default to
-                # some value which /should/ get set if its left as the default and where the default is
-                # meant to be declarative.  Instead of property_is_set? we should most likely be using
-                # nil? but we're going to deprecate all of it anyway.  Just type out what you really mean longhand.
-                #
-                if args.empty? && !block
-                  if !new_resource.property_is_set?(__method__) && current_resource
-                    Chef.deprecated(:namespace_collisions, "rename #{name} to current_resource.#{name}")
-                    return current_resource.public_send(__method__)
-                  end
-                end
-                Chef.deprecated(:namespace_collisions, "rename #{name} to new_resource.#{name}")
-                new_resource.public_send(__method__, *args, &block)
-              end
-            EOM
-          end
+          # this magic, stated simply, is that any instance method declared directly on
+          # the resource we are building, will be accessible from the action_class(provider)
+          # instance.  methods declared on Chef::Resource and properties are not inherited.
           dsl_methods =
             resource.class.public_instance_methods +
             resource.class.protected_instance_methods -
             provider_class.instance_methods -
-            resource.class.properties.keys
+            resource.class.properties.keys -
+            resource.class.properties.keys.map { |k| "#{k}=".to_sym } -
+            Chef::Resource.instance_methods
           def_delegators(:new_resource, *dsl_methods)
         end
         include @included_resource_dsl_module
diff --git a/spec/integration/recipes/resource_action_spec.rb b/spec/integration/recipes/resource_action_spec.rb
index d6ea4347c4..149b17fcad 100644
--- a/spec/integration/recipes/resource_action_spec.rb
+++ b/spec/integration/recipes/resource_action_spec.rb
@@ -378,94 +378,6 @@ module ResourceActionSpec
       end
     end
 
-    context "With a resource with property x" do
-      class ResourceActionSpecWithX < Chef::Resource
-        resource_name :resource_action_spec_with_x
-        property :x, default: 20
-        action :set do
-          # Access x during converge to ensure that we emit no warnings there
-          x
-        end
-      end
-
-      context "And another resource with a property x and an action that sets property x to its value" do
-        class ResourceActionSpecAlsoWithX < Chef::Resource
-          resource_name :resource_action_spec_also_with_x
-          property :x
-          action :set_x_to_x do
-            resource_action_spec_with_x "hi" do
-              x x
-            end
-          end
-          def self.x_warning_line
-            __LINE__ - 4
-          end
-          action :set_x_to_x_in_non_initializer do
-            r = resource_action_spec_with_x "hi" do
-              x 10
-            end
-            x_times_2 = r.x * 2
-          end
-          action :set_x_to_10 do
-            resource_action_spec_with_x "hi" do
-              x 10
-            end
-          end
-        end
-
-        attr_reader :x_warning_line
-
-        it "Using the enclosing resource to set x to x emits a warning that you're using the wrong x" do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          recipe = converge do
-            resource_action_spec_also_with_x "hi" do
-              x 1
-              action :set_x_to_x
-            end
-          end
-          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
-          expect(warnings.size).to eq 2
-          expect(warnings[0]).to match(/property x is declared in both resource_action_spec_with_x\[hi\] and resource_action_spec_also_with_x\[hi\] action :set_x_to_x. Use new_resource.x instead. At #{__FILE__}:#{ResourceActionSpecAlsoWithX.x_warning_line}/)
-        end
-
-        it "Using the enclosing resource to set x to x outside the initializer emits no warning" do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          recipe = converge do
-            resource_action_spec_also_with_x "hi" do
-              x 1
-              action :set_x_to_x_in_non_initializer
-            end
-          end
-          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
-          expect(warnings.size).to eq 1  # the deprecation warning, not the property masking one
-        end
-
-        it "Using the enclosing resource to set x to 10 emits no warning" do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          recipe = converge do
-            resource_action_spec_also_with_x "hi" do
-              x 1
-              action :set_x_to_10
-            end
-          end
-          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
-          expect(warnings.size).to eq 1  # the deprecation warning, not the property masking one
-        end
-
-        it "Using the enclosing resource to set x to 10 emits no warning" do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          recipe = converge do
-            r = resource_action_spec_also_with_x "hi"
-            r.x 1
-            r.action :set_x_to_10
-          end
-          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
-          expect(warnings.size).to eq 1  # the deprecation warning, not the property masking one
-        end
-      end
-
-    end
-
     context "With a resource with a set_or_return property named group (same name as a resource)" do
       class ResourceActionSpecWithGroupAction < Chef::Resource
         resource_name :resource_action_spec_set_group_to_nil
@@ -504,13 +416,6 @@ module ResourceActionSpec
           end
         end
       end
-
-      it "Raises an error when attempting to use a template in the action" do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        expect_converge do
-          has_property_named_template "hi"
-        end.to raise_error(/Property `template` of `has_property_named_template\[hi\]` was incorrectly passed a block. Possible property-resource collision. To call a resource named `template` either rename the property or else use `declare_resource\(:template, ...\)`/)
-      end
     end
 
     context "When a resource declares methods in action_class" do