Skip to content
This repository has been archived by the owner on Feb 7, 2022. It is now read-only.

Migration to MichelFortin's implementation. #50

Merged
merged 3 commits into from
Feb 14, 2014
Merged

Conversation

iammichiel
Copy link
Contributor

Since flydev has marked his version as deprecated (it was a simple composer port of Michel Fortin's initial project), it seems good to revert the dependency to the stable/continued project.

I have not been able to execute phpunit tests but have used this implementation in a closed-source project without trouble.

Also mentionned here : #49

Since flydev has marked his version as deprecated (it was a simple composer port of Michel Fortin's initial project), it seems good to revert the dependency to the stable/continued project.
@pilot
Copy link
Contributor

pilot commented Feb 13, 2014

@iammichiel nice! But please update test to following https://travis-ci.org/KnpLabs/KnpMarkdownBundle/jobs/18814822

Michel Fortin's implementation is able to handle the fencedCodeBlock without any escaping mecanism.
@iammichiel
Copy link
Contributor Author

I have checked out the issues from testing. One is resolved by the commit above and the other... well, it's actually up to us to make a decision. Both implementations give different results. (diff : http://diffchecker.com/kzqufgnv)

In the flydev implementation footnotes are using css classes footnote-ref and footnote-backref while the Michel Fortin implementation uses the html attributes rel and ref.

Should I update the test and add a note about it in the readme? This is possibly a BC-break...

@docteurklein
Copy link
Contributor

can we provide 2 adapters (the old one and the new one) and et the user choose ?

@pilot
Copy link
Contributor

pilot commented Feb 14, 2014

@docteurklein I think have no sense maintain not supported flydev version, as well we have a releases with old version. So BC break is OK - this is will be new release.

@iammichiel yes, please update tests

@docteurklein
Copy link
Contributor

I agree :)

iammichiel pushed a commit to iammichiel/KnpMarkdownBundle that referenced this pull request Feb 14, 2014
The footnote behaviour has changed during migration. You might want to check out the pr mentionning it : KnpLabs#50.
The footnote behaviour has changed during migration. You might want to check out the pr mentionning it : KnpLabs#50
pilot added a commit that referenced this pull request Feb 14, 2014
Migration to MichelFortin's implementation.
@pilot pilot merged commit 66e891b into KnpLabs:master Feb 14, 2014
@stof
Copy link
Contributor

stof commented Feb 20, 2014

@pilot Given that the MarkdownParserInterface has changed (the method is renamed), this should have been released as 2.0, not as 1.3 as it is not BC. Please follow semantic versionning to avoid breaking projects depending on the bundle

@stof
Copy link
Contributor

stof commented Feb 20, 2014

thus, this broke the library entirely, because usage of the parser have not been updated. I'm submitting a PR reverting the interface change

stof added a commit to stof/KnpMarkdownBundle that referenced this pull request Feb 20, 2014
All code in the bundle is still relying on the transformMarkdown method in
parsers, not on the transform method.
Normal parsers are still working because they still have the method, and
PHP does not have strict type checking for typehints.
However, the SundownParser was still broken. KnpLabs#51 renamed the method to
avoid a fatal error for the interface, but then the method used by the
bundle became missing.
pilot added a commit that referenced this pull request Feb 20, 2014
Reverted the interface renaming done in #50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants