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.

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

Removing sfYaml (unmaintained) in favor of Symfony\Yaml #43

Merged
merged 4 commits into from
Oct 2, 2018

Conversation

jaydiablo
Copy link
Member

sfYaml has been abandoned (https://github.com/fabpot-graveyard/yaml) and I'd like to make sure that it works on new versions of PHP as they are released, unfortunately Doctrine's tests don't exactly cover a lot of the sfYaml code.

I considered forking it and getting tests running on travis, but that was proving to be more effort than it was worth (and we don't really want to have to maintain it if we don't have to). So I decided to try Symfony\Yaml in place of sfYaml, which seems to be a near drop-in replacement.

Nearly... this will be a major version bump of this lib, as Symfony\Yaml does throw some errors when the yaml contains invalid syntax (in our case this was duplicate keys, un-quoted strings that start with a percent sign and extra characters outside of quotes that shouldn't be there). IMO it's a good thing that the parser catches these, but any projects using this fork of Doctrine may need to fix their schema/fixture files to work with this parser.

FWIW, I did try optionally using the PECL Yaml extension if available, but that's strictly a Yaml 1.1 parser (sfYaml and Symfony\Yaml follow the 1.2 spec, specifically when it comes to converting values to boolean (i.e. unquoted Yes/No). The 1.2 parsers don't do this, but the 1.1 parsers do). I felt this was a big enough difference to not suggest/use the PECL Yaml parser here.

@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #43 into master will increase coverage by 0.5%.
The diff coverage is 40%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master      #43     +/-   ##
===========================================
+ Coverage     69.66%   70.16%   +0.5%     
+ Complexity     7816     7607    -209     
===========================================
  Files           265      261      -4     
  Lines         19054    18561    -493     
===========================================
- Hits          13274    13024    -250     
+ Misses         5780     5537    -243
Impacted Files Coverage Δ Complexity Δ
lib/Doctrine/Core.php 60.81% <ø> (+0.36%) 97 <0> (-1) ⬇️
lib/Doctrine/Compiler.php 0% <0%> (ø) 14 <0> (-1) ⬇️
lib/Doctrine/Parser/Yml.php 53.84% <50%> (ø) 4 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad555f0...eb766df. Read the comment docs.

@jaydiablo jaydiablo requested a review from a team September 24, 2018 22:45
@jaydiablo jaydiablo merged commit 25df2f3 into diablomedia:master Oct 2, 2018
@jaydiablo jaydiablo deleted the no-sfyaml branch October 2, 2018 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants