🗑️ Deprecate backward compatible parameters to new and starttls

lib/net/imap.rb

@@ -760,7 +760,7 @@ class << self
     # [idle_response_timeout]
     #   Seconds to wait until an IDLE response is received
     #
-    # See DeprecatedClientOptions for obsolete backwards compatible arguments.
+    # See DeprecatedClientOptions.new for deprecated arguments.
     #
     # ==== Examples
     #
@@ -810,16 +810,15 @@ class << self
     # [Net::IMAP::ByeResponseError]
     #   Connected to the host successfully, but it immediately said goodbye.
     #
-    def initialize(host, options = {}, *deprecated)
+    def initialize(host, port: nil, ssl:  nil,
+                   open_timeout: 30, idle_response_timeout: 5)
       super()
-      options = convert_deprecated_options(options, *deprecated)
-
       # Config options
       @host = host
-      @port = options[:port] || (options[:ssl] ? SSL_PORT : PORT)
-      @open_timeout = options[:open_timeout] || 30
-      @idle_response_timeout = options[:idle_response_timeout] || 5
-      @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(options[:ssl])
+      @port = port || (ssl ? SSL_PORT : PORT)
+      @open_timeout = Integer(open_timeout)
+      @idle_response_timeout = Integer(idle_response_timeout)
+      @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(ssl)
 
       # Basic Client State
       @utf8_strings = false
@@ -1076,7 +1075,12 @@ def logout
     # Sends a {STARTTLS command [IMAP4rev1 §6.2.1]}[https://www.rfc-editor.org/rfc/rfc3501#section-6.2.1]
     # to start a TLS session.
     #
-    # Any +options+ are forwarded to OpenSSL::SSL::SSLContext#set_params.
+    # Any +options+ are forwarded directly to
+    # {OpenSSL::SSL::SSLContext#set_params}[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html#method-i-set_params];
+    # the keys are names of attribute assignment methods on
+    # SSLContext[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html].
+    #
+    # See DeprecatedClientOptions#starttls for deprecated arguments.
     #
     # This method returns after TLS negotiation and hostname verification are
     # both successful.  Any error indicates that the connection has not been
@@ -1098,14 +1102,8 @@ def logout
     # Server capabilities may change after #starttls, #login, and #authenticate.
     # Cached #capabilities will be cleared when this method completes.
     #
-    def starttls(options = {}, verify = true)
-      begin
-        # for backward compatibility
-        certs = options.to_str
-        options = create_ssl_params(certs, verify)
-      rescue NoMethodError
-      end
-      @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(options || {})
+    def starttls(**options)
+      @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(options)
       send_command("STARTTLS") do |resp|
         if resp.kind_of?(TaggedResponse) && resp.name == "OK"
           clear_cached_capabilities
@@ -2354,20 +2352,6 @@ def remove_response_handler(handler)
 
     @@debug = false
 
-    def convert_deprecated_options(
-      port_or_options = {}, usessl = false, certs = nil, verify = true
-    )
-      port_or_options.to_hash
-    rescue NoMethodError
-      # for backward compatibility
-      options = {}
-      options[:port] = port_or_options
-      if usessl
-        options[:ssl] = create_ssl_params(certs, verify)
-      end
-      options
-    end
-
     def start_imap_connection
       @greeting        = get_server_greeting
       @capabilities    = capabilities_from_resp_code @greeting
@@ -2695,23 +2679,6 @@ def build_ssl_ctx(ssl)
       end
     end
 
-    def create_ssl_params(certs = nil, verify = true)
-      params = {}
-      if certs
-        if File.file?(certs)
-          params[:ca_file] = certs
-        elsif File.directory?(certs)
-          params[:ca_path] = certs
-        end
-      end
-      if verify
-        params[:verify_mode] = VERIFY_PEER
-      else
-        params[:verify_mode] = VERIFY_NONE
-      end
-      return params
-    end
-
     def start_tls_session
       raise "SSL extension not installed" unless defined?(OpenSSL::SSL)
       raise "already using SSL" if @sock.kind_of?(OpenSSL::SSL::SSLSocket)
@@ -2746,3 +2713,6 @@ def self.saslprep(string, **opts)
 require_relative "imap/response_data"
 require_relative "imap/response_parser"
 require_relative "imap/authenticators"
+
+require_relative "imap/deprecated_client_options"
+Net::IMAP.prepend Net::IMAP::DeprecatedClientOptions

lib/net/imap/deprecated_client_options.rb

@@ -0,0 +1,139 @@
+# frozen_string_literal: true
+
+module Net
+  class IMAP < Protocol
+
+    # This module handles deprecated arguments to various Net::IMAP methods.
+    module DeprecatedClientOptions
+
+      # :call-seq:
+      #   Net::IMAP.new(host, **options) # standard keyword options
+      #   Net::IMAP.new(host, options)   # obsolete hash options
+      #   Net::IMAP.new(host, port)      # obsolete port argument
+      #   Net::IMAP.new(host, port, usessl, certs = nil, verify = true) # deprecated SSL arguments
+      #
+      # Translates Net::IMAP.new arguments for backward compatibility.
+      #
+      # ==== Obsolete arguments
+      #
+      # Using obsolete arguments does not a warning.  Obsolete arguments will be
+      # deprecated by a future release.
+      #
+      # If a second positional argument is given and it is a hash (or is
+      # convertable via +#to_hash+), it is converted to keyword arguments.
+      #
+      #     # Obsolete:
+      #     Net::IMAP.new("imap.example.com", options_hash)
+      #     # Use instead:
+      #     Net::IMAP.new("imap.example.com", **options_hash)
+      #
+      # If a second positional argument is given and it is not a hash, it is
+      # converted to the +port+ keyword argument.
+      #     # Obsolete:
+      #     Net::IMAP.new("imap.example.com", 114433)
+      #     # Use instead:
+      #     Net::IMAP.new("imap.example.com", port: 114433)
+      #
+      # ==== Deprecated arguments
+      #
+      # Using deprecated arguments prints a warning.  Convert to keyword
+      # arguments to avoid the warning.  Deprecated arguments will be removed in
+      # a future release.
+      #
+      # If +usessl+ is false, +certs+, and +verify+ are ignored.  When it true,
+      # all three arguments are converted to the +ssl+ keyword argument.
+      # Without +certs+ or +verify+, it is converted to <tt>ssl: true</tt>.
+      #     # DEPRECATED:
+      #     Net::IMAP.new("imap.example.com", nil, true) # => prints a warning
+      #     # Use instead:
+      #     Net::IMAP.new("imap.example.com", ssl: true)
+      #
+      # When +certs+ is a path to a directory, it is converted to <tt>ca_path:
+      # certs</tt>.
+      #     # DEPRECATED:
+      #     Net::IMAP.new("imap.example.com", nil, true, "/path/to/certs") # => prints a warning
+      #     # Use instead:
+      #     Net::IMAP.new("imap.example.com", ssl: {ca_path: "/path/to/certs"})
+      #
+      # When +certs+ is a path to a file, it is converted to <tt>ca_file:
+      # certs</tt>.
+      #     # DEPRECATED:
+      #     Net::IMAP.new("imap.example.com", nil, true, "/path/to/cert.pem") # => prints a warning
+      #     # Use instead:
+      #     Net::IMAP.new("imap.example.com", ssl: {ca_file: "/path/to/cert.pem"})
+      #
+      # When +verify+ is +false+, it is converted to <tt>verify_mode:
+      # OpenSSL::SSL::VERIFY_NONE</tt>.
+      #     # DEPRECATED:
+      #     Net::IMAP.new("imap.example.com", nil, true, nil, false) # => prints a warning
+      #     # Use instead:
+      #     Net::IMAP.new("imap.example.com", ssl: {verify_mode: OpenSSL::SSL::VERIFY_NONE})
+      #
+      def initialize(host, port_or_options = nil, *deprecated, **options)
+        if port_or_options.nil? && deprecated.empty?
+          super host, **options
+        elsif options.any?
+          # Net::IMAP.new(host, *__invalid__, **options)
+          raise ArgumentError, "Do not combine deprecated and keyword arguments"
+        elsif port_or_options.respond_to?(:to_hash) and deprecated.any?
+          # Net::IMAP.new(host, options, *__invalid__)
+          raise ArgumentError, "Do not use deprecated SSL params with options hash"
+        elsif port_or_options.respond_to?(:to_hash)
+          super host, **Hash.try_convert(port_or_options)
+        elsif deprecated.empty?
+          super host, port: port_or_options
+        elsif deprecated.shift
+          warn "DEPRECATED: Call Net::IMAP.new with keyword options", uplevel: 1
+          super host, port: port_or_options, ssl: create_ssl_params(*deprecated)
+        else
+          warn "DEPRECATED: Call Net::IMAP.new with keyword options", uplevel: 1
+          super host, port: port_or_options, ssl: false
+        end
+      end
+
+      # :call-seq:
+      #   starttls(**options) # standard
+      #   starttls(options = {}) # obsolete
+      #   starttls(certs = nil, verify = true) # deprecated
+      #
+      # Translates Net::IMAP#starttls arguments for backward compatibility.
+      #
+      # Support for +certs+ and +verify+ will be dropped in a future release.
+      #
+      # See ::new for interpretation of +certs+ and +verify+.
+      def starttls(*deprecated, **options)
+        if deprecated.empty?
+          super(**options)
+        elsif options.any?
+          # starttls(*__invalid__, **options)
+          raise ArgumentError, "Do not combine deprecated and keyword options"
+        elsif deprecated.first.respond_to?(:to_hash) && deprecated.length > 1
+          # starttls(*__invalid__, **options)
+          raise ArgumentError, "Do not use deprecated verify param with options hash"
+        elsif deprecated.first.respond_to?(:to_hash)
+          super(**Hash.try_convert(deprecated.first))
+        else
+          warn "DEPRECATED: Call Net::IMAP#starttls with keyword options", uplevel: 1
+          super(**create_ssl_params(*deprecated))
+        end
+      end
+
+      private
+
+      def create_ssl_params(certs = nil, verify = true)
+        params = {}
+        if certs
+          if File.file?(certs)
+            params[:ca_file] = certs
+          elsif File.directory?(certs)
+            params[:ca_path] = certs
+          end
+        end
+        params[:verify_mode] =
+          verify ? OpenSSL::SSL::VERIFY_PEER : OpenSSL::SSL::VERIFY_NONE
+        params
+      end
+
+    end
+  end
+end

test/net/imap/fake_server/test_helper.rb

@@ -4,10 +4,14 @@
 
 module Net::IMAP::FakeServer::TestHelper
 
-  def run_fake_server_in_thread(timeout: 5, **opts)
+  def run_fake_server_in_thread(ignore_io_error: false, timeout: 5, **opts)
     Timeout.timeout(timeout) do
       server = Net::IMAP::FakeServer.new(timeout: timeout, **opts)
-      @threads << Thread.new do server.run end if @threads
+      @threads << Thread.new do
+        server.run
+      rescue IOError
+        raise unless ignore_io_error
+      end
       yield server
     ensure
       server&.shutdown