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

Lambdasoup eats the doctype #32

Closed
copy opened this issue Apr 19, 2020 · 16 comments
Closed

Lambdasoup eats the doctype #32

copy opened this issue Apr 19, 2020 · 16 comments

Comments

@copy
Copy link
Contributor

copy commented Apr 19, 2020

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
Copy link
Owner

aantron commented Apr 20, 2020

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 added a commit that referenced this issue Apr 20, 2020
@aantron
Prepend <!DOCTYPE html> to full documents
2758b31 
See #32.
@aantron
Copy link
Owner

aantron commented Apr 20, 2020

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

@copy
Copy link
Contributor Author

copy commented Apr 20, 2020

Yes, current master fixes the issue, thanks!

It seems to have two minor problems though:

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

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

@aantron aantron mentioned this issue Apr 22, 2020
Merged
@aantron
Copy link
Owner

aantron commented Apr 22, 2020

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.

@aantron aantron closed this as completed Apr 22, 2020
@copy
Copy link
Contributor Author

copy commented Apr 22, 2020

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
Copy link
Contributor

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?

@dmbaturin dmbaturin mentioned this issue May 31, 2020
Closed
@aantron
Copy link
Owner

aantron commented Sep 20, 2020

@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
Copy link
Contributor Author

copy commented Sep 20, 2020

@aantron That sounds good to me.

@dmbaturin
Copy link
Contributor

@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
Copy link
Owner

aantron commented Sep 21, 2020

@dmbaturin, I have two other suggestions:

  • Provide some functions to manipulate the doctype on a soup node.
  • Provide a completely separate mechanism for parsing doctypes from the front of strings, and emitting them. So a user that wants to extract the doctype from the input would, separately from calling parse, also call read_doctype, and get a doctype value which can then be emitted (and somehow analyzed).

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 added a commit that referenced this issue Oct 9, 2020
@aantron
Store doctype in soup nodes
d066d16 
Second attempt at #32.
Closes #33.
@aantron
Copy link
Owner

aantron commented Oct 9, 2020

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
Copy link
Contributor

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

@aantron
Copy link
Owner

aantron commented Oct 12, 2020

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

@dmbaturin
Copy link
Contributor

@aantron The new version will be 0.8.0?

@aantron
Copy link
Owner

aantron commented Oct 16, 2020

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 aantron mentioned this issue Oct 19, 2020
Merged
@aantron
Copy link
Owner

aantron commented Oct 20, 2020

0.7.2 is now available in opam.

@dmbaturin dmbaturin mentioned this issue Oct 21, 2020
Closed
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
Development

No branches or pull requests
3 participants
@dmbaturin @copy @aantron