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.

Sign up for GitHub

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

Jump to bottom

Calling ActiveRecord::Base.send(:include, PAK::ValidatesHostname) has side effects #10

Open
pgouv opened this issue Feb 15, 2018 · 6 comments · May be fixed by #12
Open

Calling ActiveRecord::Base.send(:include, PAK::ValidatesHostname) has side effects #10

pgouv opened this issue Feb 15, 2018 · 6 comments · May be fixed by #12

Comments

@pgouv
Copy link

pgouv commented Feb 15, 2018

I was wondering why Rails.application.config.active_record.belongs_to_required_by_default was not working and this gem is the culprit.
I am wondering whether there is a fix for this.
@KimNorgaard
Copy link
Owner

I have not been working with Rails for ages and currently do not have a lot of time on my hands. Any help fixing this issue would be much appreciated.

@schwern schwern mentioned this issue Feb 17, 2020
Closed
schwern added a commit to evalEmpire/validates_hostname that referenced this issue Feb 17, 2020
@schwern
feature: Change to a Rails 4 ActiveModel validator.
1dc857e 
Rails 4 introduced ActiveModel, ActiveModel::Validations and
ActiveModel::EachValidator.

Instead of injecting ourselves into ActiveRecord::Base, we will be
automatically picked up. This fixes KimNorgaard#10.

We can now also be used on ActiveModel as well as ActiveRecord. This
simplifies testing.

Also
* rspec-rails was dropped becasue it seems to want all of Rails loaded.
  We only needed one convenience method, errors_on. It has been copied into our test model.
* Bumped our runtime dependency to activemodel >= 4.0. Dropped the others.
* Dropped the development dependency on all of Rails. We only need ActiveModel.
@schwern schwern linked a pull request Feb 17, 2020 that will close this issue
Open
@schwern
Copy link

schwern commented Feb 17, 2020

@KimNorgaard I'm interested in adopting this gem. I keep having the need to do domain validation in Rails and this is the best validator I've found.

Features I'd like to add:

  • label validator
  • option to restrict by number of labels (ie. foo.com vs www.foo.com)
  • look into using PublicSuffix
  • disallow local and private IPs and hosts (example.com, localhost, 127.0.0.1, etc)

@KimNorgaard
Copy link
Owner

Thank you for reaching out and offering help. I'm still a bit pressed on time but I'm willing to give you a commit bit and assist with pushing to rubygems when we have a stable version.

@KimNorgaard
Copy link
Owner

KimNorgaard commented Feb 18, 2020 •
edited
Loading

I just pushed v1.0.10 with the bugfix from #13 to rubygems. Since your changes are breaking backwards compatibility I'd like to move to wards v1.1.0 or possibly v2.0.0. What are your thoughts?

@schwern
Copy link

schwern commented Feb 26, 2020

Thanks! I only have a couple years of Ruby experience, and this is my first time messing with a gem. I cribbed from another validation gem. I do have a lot of experience releasing Perl modules.

Yes, v2.0.0 makes sense if we're breaking backwards compatibility and following semantic versioning. If you have any other compatibility breaking changes you'd like to make while we're at it, perhaps you can write them up as issues and I can have a look.

@KimNorgaard
Copy link
Owner

My experience with releasing gems is also limited to this one, so I guess we are in the same boat.
It's a pretty simple validator and I don't expect it to change much since it's pretty much based off the RFCs and they also don't change much. I don't have any big plans for a v2.

I once thought about doing something clever with the ALLOWED_TLDS list, specifically making it always updated, but the only way that would work would be fetching the list runtime which I'm against for obvious reasons. For now I've just updated it by hand periodically and bumped the version. I guess that suffices for most people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@schwern @KimNorgaard @pgouv