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

Fixes issue #44 & replaces saveHTML with saveXML for php versions preceding 5.3.6 #57

Closed
wants to merge 0 commits into from
Closed

Fixes issue #44 & replaces saveHTML with saveXML for php versions preceding 5.3.6 #57

wants to merge 0 commits into from

Conversation

acmitch
Copy link

@acmitch acmitch commented Apr 6, 2015

The issue here is that when using $DOMDocument->saveHTML($Node) the node paramerter in saveHTML was added in 5.3.6 therefore breaking previous versions.

Since only working with subsets of the document you can achieve the same effect by using saveXML and passing in the Node parameter.

@erusev
Copy link
Owner

erusev commented Apr 6, 2015

I believe that previous versions used to use saveXML instead of saveHTML, and I don't remember what they were, but there were issues with this.

@acmitch
Copy link
Author

acmitch commented Apr 6, 2015

For me, the pros might outway the cons. I can't seem to include any HTML in my Markdown files or the HTML content will not render. I have no control over the server to update the PHP version so I will use my branch for now, but I'm sure others will run into similar issues. I'd be curious to know what the issues were if you can remember, so I can be on the lookout.

@acmitch
Copy link
Author

acmitch commented Apr 7, 2015

Made another commit on this that fixes issue #44

# remove <html><body></body></html> 
$doc->replaceChild($doc->firstChild->firstChild->firstChild, $doc->firstChild);

The above code works only if the <body> has only one child node. When two HTML tags are smushed together, multiple support is needed.

I expanded on the processTag function by creating a parent function called processTags which handles multiple instances. Might want to run some test making sure the fix works in all cases and doesn't negatively affect speed.

@acmitch
Copy link
Author

acmitch commented Apr 7, 2015

One last comment on this, sorry about the chatter. I think I have most the kinks worked out on this fix, hope it saves you guys some time.

I do want to point out a couple of things:
It seems like the main issue is that classes cannot be assigned to elements that have nested markdown, even with the Markdown="1" set. What goes wrong? Well, first it strips the inner text of the classed element and then builds that text inside a separate element.

However, it seems like the issue lies in the Parsedown library, not the ParsedownExtra library. I'd even go as far as to say that once you figure out that issue the Markdown="1" will be obsolete. You could just call $elementText = "\n".$this->text($elementText)."\n"; from the ParsedownExrta library every time you needed to process text (any element that is not an instance of DOMElement and not inside the textLevelElements array ).

Also, please add a label tag to the Parsedown textLevelElements array.

@acmitch acmitch changed the title Replaces saveHTML with saveXML for php versions preceding 5.3.6 Fixes issue #44 & replaces saveHTML with saveXML for php versions preceding 5.3.6 Apr 7, 2015
@erusev
Copy link
Owner

erusev commented Apr 7, 2015

@acmitch Thanks for the feedback. I'll review as soon as I have some time.

@acmitch acmitch closed this Apr 7, 2015
@Lomanic Lomanic mentioned this pull request Sep 15, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acmitch @erusev