tenderlove / rails_autolink

The auto_link function from Rails
MIT License
589 stars 91 forks source link

future of rails_autolink #55

Open jjb opened 9 years ago

jjb commented 9 years ago

I think the initial goal of rails_autolink was to provide the functionality that was removed from rails. To that end it has served its purpose well.

Is there any interest from the maintainers in keeping the project going and advancing it? We can make API- and behavior-changing improvements for the next version, and also improve the implementation.

This project, even thought it's pretty simple and small, still seems to do the best job of parsing text for a wide variety of URLs in user-entered content.

Here are things I would like to see improved.

more modular code and, by extension, features

For example, the project provides a good link detector/extractor. I'm "hacking" it to use this functionality in a standalone manner.

@urls = []
Autolinker.instance.auto_link(text, link: :urls) do |url|
  @urls << url
  ""
end

If there were official support for this feature, it could have a nicer API and also be more efficient.

dealing with html vs. non-html input

This project very ambitiously detects URLs that are already within a tags and doesn't link them again. I don't know what the use case is for dealing with content that might have a mix of linked and not-linked URLs, but I certainly don't have one. Because of this complicated code, it makes it more difficult to add other features. I would propose either making support for html input an optional, separate phase, or, I prefer, removing it entirely, if we poll current users and find that this feature is not commonly used.

more correct notion of "sanitization"

Currently sanitization will both remove unapproved html tags, and also html-escape all (I believe) the output. But what if I don't want to worry about unsafe html tags, but I do want the output (in particular, the values of the "href" attribute) to be html-escaped? There's no way to do this.

Thoughts?

If the maintainers think that some or all of these are ideas are appropriate for rails_autolink 2, maybe we can make a branch and I'll start submitting patches.

/cc @xuanxu @tenderlove @tardate

tenderlove commented 9 years ago

I am not interested in maintaining this library. If someone else wants it, please take over!

coreyhaines commented 9 years ago

If someone does take it, could we rename it tender_autolinking ?

FWIW, I end up using this on almost all my rails-based apps. I use it for the very simple, focused usage of auto-linking text in my view.

tenderlove commented 9 years ago

@coreyhaines well now you have commit access!

jjb commented 9 years ago

I'd also be interested in being a maintainer

@coreyhaines what do you think of my vision described above?

coreyhaines commented 9 years ago

@jjb A couple comments.

  1. Dealing with html input

This is actually a case that I do run into. I have had times where I want to support people putting in html, but still running through this for auto-linking of URLs. I have actually looked that deeply into the code, but if the goal of this is to make it a bit more easy to add features, I'd be happy to chat over a good cleanup PR.

  1. Using it as a link extractor.

I'd be really nervous expanding the purpose of the gem. To be honest, I'd be inclined to remove things. :) Perhaps extract that code out into a separate gem?

  1. More correct notion of "sanitization"

A PR with some examples of it would be cool.

coreyhaines commented 9 years ago

To be honest, since @tenderlove isn't really looking to maintain it, I wonder if it might be time to just fork it. My original thought was to fork it, change the name to tender_autolinking, then look through the code to see if there is anything to remove. :)

@jjb Seeing how you have some thoughts on extending the use cases for this, perhaps a fork to move towards a link extractor?

jjb commented 9 years ago

@coreyhaines

mixed input

gotcha. i suppose one might allow users to bold text but the user would expect raw urls to be clickable.

link extractor

well, the code is already doing this internally, so it's sort of a hidden useful feature. this project is one of the most thorough/permissive detectors of links out there. so yeah, it could be another gem.

sanitize

consider this

auto_link("http://www.rubyonrails.org <script>alert('Script!')</script> I A->B <3 ruby")
=> "<a href=\"http://www.rubyonrails.org\">http://www.rubyonrails.org</a>  I A->B &lt;3 ruby"

here it's doing all these three things

  1. removing unsafe tags
  2. html encoding a <
  3. not escaping the >

1 and 2 i'd like to be done as separate options, and the escaping isn't thorough/smart, resulting in 3

i suppose these issues are a limitation of rails's sanitize, and it's reasonable for autolink to simply depend on it and allow it to be turned off, so maybe this isn't worth working on.

but this gets back to the mixed input issue -- if we knew that users were always not entering html, then we could more confidently and thoroughly escape/strip the input and autolink it at the same time

jjb commented 9 years ago

@coreyhaines do you have any new features or behavior changes that you want to work on?

coreyhaines commented 9 years ago

Personally I don't. For all the use cases I've had for autolinking, the gem is pretty complete.

There are a couple PRs that could stand being reviewed. We could then look to merge some of those.

This issue https://github.com/tenderlove/rails_autolink/issues/50 seems like something you could work on, if you are interested. Autolinking twitter handles seems pretty useful.

coreyhaines commented 9 years ago

@jjb

Mixed Input

The case you mention where a user can put in a <b> tag is exactly the case I've used. :)

Link Extractor

The main thought I have is that expanding the scope of this gem from autolinking to parsing/extracting-for-processing links might be confusing.

Sanitize

My opinion is that it would be more trouble than it is worth to break from using the underlying rails sanitizer. Like you say, it probably is worth it to just keep it as-is with an option to turn off sanitize.

jjb commented 9 years ago

Thanks for your response. The twitter handle autolinking isn't something I'd need. In general I'm wary of investing time into the gem if it continues to support mixed input, because I'm gun shy from previous hours wasted when that code was getting in my way. I wish I could remember what the case was, it was a few months ago. Maybe in the future I'll put together an example and can have a more rational discussion :)

teeparham commented 7 years ago

I made some changes to rails_autolink & released it as the anchored gem: https://github.com/neighborland/anchored.

The differences are:

jjb commented 7 years ago

@teeparham interesting, will take a look!