search

aantron / lambdasoup

Functional HTML scraping and rewriting with CSS in OCaml
https://aantron.github.io/lambdasoup
MIT License
380 stars 31 forks source link

Lambdasoup eats the doctype #32

Closed copy closed 4 years ago

copy commented 4 years ago

I'm doing some rewriting similar to postprocess.ml and found that either Soup.parse or Soup.to_string remove the doctype.

utop [0]: #require "lambdasoup";;
utop [1]: Soup.parse "<!doctype html><html><body><b>Hi</b></body></html>" |> Soup.to_string;;
val _1 : string = "<html><head></head><body><b>Hi</b></body></html>"

The online docs created by postprocess.ml don't have a doctype either.

aantron commented 4 years ago

It is to_string (and pretty_print). Lambda Soup should probably check if the top-level element is <html> (as opposed to something else, indicating a fragment), and prepend a doctype on output.

aantron commented 4 years ago

Does the attached commit, which is now in master, address the issue for your usage?

copy commented 4 years ago

Yes, current master fixes the issue, thanks!

It seems to have two minor problems though:

None of the two affect me, but could cause some surprise for other users

aantron commented 4 years ago

This is now in opam in lambdasoup 0.7.1.

  • It doesn't reproduce the original doctype

Do you mean the casing? Or the presence/absence of the doctype?

  • It doesn't work if the (technically optional) html tag is not present

Lambda Soup would need to distinguish explicitly between full documents and fragments to handle that intelligently. Also, <html> can be absent in the input stream, but it is not optional in the DOM. The parser also inserts the tag whenever it is sure it is parsing a full document, even if it is absent. Otherwise, Lambda Soup assumes it is handling a fragment.

Of course, a user can manipulate the tree with the intention of having a complete document, and not add/maintain an <html> tag.

In all cases, there is the escape hatch of using Soup.signals and manually adding or removing the doctype.

We can solve any further issues when they come up.

copy commented 4 years ago

Do you mean the casing? Or the presence/absence of the doctype?

Also non-html5 doctypes, although those are probably even less common than missing html tags.

We can solve any further issues when they come up.

Agreed. Cheers for the quick fix.

dmbaturin commented 4 years ago

Guys, I have no choice but to go passive aggressive now. There's a list of reverse dependencies in the opam page. You could easily check how people use the Soup.pretty_print function and see that they are explicitly compensating for the missing doctype.

See [1] and [2].

You could consider that after this change, their code will produce nonsensical pages with a duplicate doctype. Hell, you could at least mention the maintainers of those projects in the issue and ask their opinions.

Instead you chose to silently break compatibility and leave them without even an option to disable this behaviour or specify their own doctype.

@aantron I'm very grateful to you for creating and maintaining lambdasoup. Soupault would never be possible without your work. But for goodness sake, why couldn't this be an optional argument at least?

aantron commented 3 years ago

@dmbaturin, @copy, what about an approach where Lambda Soup saves the doctype, if present, in the top-level soup node, and emits it on serialization?

This makes at least some kind of sense to me, as the soup node represents the whole document. @dmbaturin, would you still want to suppress it?

copy commented 3 years ago

@aantron That sounds good to me.

dmbaturin commented 3 years ago

@aantron There may be valid reasons to supress the original doctype and supply your own. For example, if you are adding HTML5 elements to user-supplied pages, it makes sense to force the doctype to HTML5 because user's original doctype could be XHTML 1.0 for example, and we just purposely broke XHTML compatibility.

Something like a ~keep_doctype:true argument will solve both of these issues. I think it should be true by default.

aantron commented 3 years ago

@dmbaturin, I have two other suggestions:

I'm trying to decide which approach is the least "magical" and least awkward. Ideally, Lambda Soup's behavior would remain simple, and the APIs would also remain simple for people that don't want to bother thinking about the doctype at all.

aantron commented 3 years ago

The commit linked above stores the doctype in the soup node, if the doctype was present.

If the doctype was present and one would like to forcibly drop it, it is possible to do so in this version by selecting the top-level elements from the document and serializing those instead of the document, for example with soup $ "html":

    ("doctype" >:: fun _ ->
      assert_equal
        ("<html></html>" |> parse |> to_string)
        "<html><head></head><body></body></html>";

      assert_equal
        ("<!DOCTYPE html><html></html>" |> parse |> to_string)
        "<!DOCTYPE html><html><head></head><body></body></html>";

      assert_equal
        ("<!DOCTYPE html><html></html>" |> parse $ "html" |> to_string)
        "<html><head></head><body></body></html>");

This isn't obvious, but I decided to defer documenting it until someone asks about it. I also decided to defer adding manipulators for the doctype until they are needed by someone.

I believe the above three cases cover all your needs, @copy and @dmbaturin, as I understood them, and are fairly intuitive. Please let me know if that is not the case.

dmbaturin commented 3 years ago

@aantron I think it's a sensible approach, thanks! When do you plan to make an opam release?

aantron commented 3 years ago

I'll release this "very soon" (today or tomorrow). In fact, your feedback was the last thing remaining to get before doing it :)

dmbaturin commented 3 years ago

@aantron The new version will be 0.8.0?

aantron commented 3 years ago

Probably 0.7.2. Sorry about the delay, I had to switch computers and haven't had time to set up. Planning for next week.

aantron commented 3 years ago

0.7.2 is now available in opam.