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

Remove all YAML references #5932

Merged
merged 5 commits into from Jul 13, 2016

Conversation

Projects
None yet
@lcobucci
Member

lcobucci commented Jul 12, 2016

@guilhermeblanco I think this covers everything except the documentation files, should we also change them?

lcobucci added some commits Jul 12, 2016

Removing Doctrine ORM 1 schema conversion tool.
This tool reads and writes YAML files so it should be removed.
@guilhermeblanco

This comment has been minimized.

Member

guilhermeblanco commented Jul 13, 2016

@lcobucci Documentation will be revisited once we get close to an alpha release. For now we're safe to experiment/implement stuff.

@guilhermeblanco guilhermeblanco merged commit d0d4c6d into doctrine:develop Jul 13, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@@ -1,5 +1,10 @@
# Upgrade to 3.0
## BC Break: Removed ``YAML`` mapping drivers.
If your code relies on ``YamlDriver`` or ``SimpleYamlDriver``, you should change to

This comment has been minimized.

@afoeder

afoeder Jul 13, 2016

Contributor

shouldn't that rather be

must change to

?

This comment has been minimized.

@Ocramius

@lcobucci lcobucci deleted the lcobucci:3.0-dev/remove-yaml branch Jul 13, 2016

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Jul 14, 2016

I love the cleanup! GJ

@Ocramius

This comment has been minimized.

Member

Ocramius commented Jul 19, 2016

Since everyone keeps asking, and a lot of YAML afficionados keep poking me on other channels, I'm clarifying the reasoning for this change.

  1. @guilhermeblanco is rewriting the internals of the mapping component, both for performance and for type-safety/ease of usage
  2. due to the mapping internal changes, mapping drivers would have required constant re-adapting. We simply don't have the throughput to deal with that, so we decided to get rid of some code/functionality instead, reducing the area of work.
  3. we are aiming for less mapping drivers to maintain ourselves. If YAML is such a big deal, then somebody may write a driver for it once the metadata changes are complete. It would not live in the Doctrine\ namespace though.
  4. we aim at unifying mapping drivers via https://webmozart.io/blog/2014/10/24/defining-php-annotations-in-xml/ (not yet there, but that's the aim)
  5. Over the years, we got hundreds of help requests due to a missing key, a lacking indentation level, etc. These are not important problems, they are just noise, and given the limited amount of time that all of us have, we identified the noise as the actual problem. Getting rid of an "unfit-for-work" format seemed like a better decision.

If you want to use the develop branch, consider using the orm:convert-mapping command to translate your YML definitions into XML for now.

@mickaelandrieu

This comment has been minimized.

mickaelandrieu commented Jul 19, 2016

@Ocramius thanks for the complete explanation.

So, to sum up what format should be used: php annotations (please no :( ) or xml files ? (Yay!).

Mickaël

@Ocramius

This comment has been minimized.

Member

Ocramius commented Jul 19, 2016

@michael-lavaveshkul annotations are good for in-house projects, please use XML for anything reusable/published.

@GeeH

This comment has been minimized.

Contributor

GeeH commented Jul 19, 2016

@Ocramius will you still be supporting CSV please?

@Ocramius

This comment has been minimized.

Member

Ocramius commented Jul 19, 2016

@Ocramius will you still be supporting CSV please?

I hear that IDE support is poor, so we won't do it.

@GeeH

This comment has been minimized.

Contributor

GeeH commented Jul 19, 2016

Touchè :trollface:

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Jul 19, 2016

@Ocramius Thank you, explanation is always great.

@guilhermeblanco

This comment has been minimized.

Member

guilhermeblanco commented Jul 19, 2016

I'm gonna expand the work that is being done on develop branch, so any adventurous person won't shoot me on street.

Quote strategy is mostly gone, since it's not up to the ORM handle discrepancies on user's database mapping. If you define your columns and provide the column name all lowercase, it's not up to us fix it on your Oracle driver and make it uppercased. Instead of handling this for the user, which makes our code harder to maintain and also introduce serious WTF bugs (anyone interested to see some, read this method: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php#L233 ), we decided to leverage an easier way for users to define their own NamingStrategy. People are welcome to implement their own, solving their own casing problems.

Right now, I commented out ALL support for Embeddables. Every tiny bit I was changing was revealing random bugs, which hint a bad implementation under the hood. We'll bring it back before first alpha in a new, deeply integrated support, but for now, no go.

All 300's unit tests that are failing are due to quoting being now enforced everywhere. I dropped the backtick support and every single column, table, schema, etc is now quoted. I have yet to find a solution that makes quote comparisons proper and work across all drivers when running the unit tests.

@BallisticPain

This comment has been minimized.

BallisticPain commented Jul 19, 2016

I've never been a fan of XML files, but I understand where your decision is coming from. I haven't used the YAML mapping since the introduction of annotations. I'll keep in mind if I ever release a library it's preferred to be in XML... why is that though?

I know previous reasons for XML vs YAML is validation, but YAML now can be validated I thought... so just curious.

Thanks for all of your hard efforts!

@Ocramius

This comment has been minimized.

Member

Ocramius commented Jul 19, 2016

YAML now can be validated I thought... so just curious.

We'd have to write the validation logic for it, and that is just overhead.

@theofidry

This comment has been minimized.

theofidry commented Jul 19, 2016

@guilhermeblanco nobody is gonna shot you in the street because you made a sensible decisions ;)

I really like YAML so I belongs to the person disappointed by this decision, but I perfectly understand the rationale so thanks very much for the explanation and for working so hard for Doctrine 3 👍 👌

@BallisticPain

This comment has been minimized.

BallisticPain commented Jul 19, 2016

@Ocramius
Fully understand, I'd rather the focus be on what y'all do best than the ancillary parts. Those that really want the YAML support can create an additional library to add that support with proper focus on the YAML.

Now why is it that XML is the standard requested for third-party available libraries? Is it because it's so verbose?

@theofidry

This comment has been minimized.

theofidry commented Jul 19, 2016

@BallisticPain XML it is a tool of choice for configuration as you can validate your config against an XSD schema. For example when using PHPUnit, if you have:

<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/5.2/phpunit.xsd">

You'll be able to have syntax highlighting and auto-completion when writing your PHPUnit file.

@Ocramius

This comment has been minimized.

Member

Ocramius commented Jul 19, 2016

@BallisticPain what @theofidry said. In addition to that, should we change our schema to remove configs that we don't support, or alter how they are to be set up, you'd get immediate feedback from the XSD validation (which would fail, highlighting the issue).

Note: we're not talking about science-fiction here, this is actually stuff that already works out of the box in editors such as Eclipse, Netbeans or PHPStorm.

@BallisticPain

This comment has been minimized.

BallisticPain commented Jul 19, 2016

@theofidry @Ocramius
Thanks! That makes a lot of sense. I just usually try to stay away from XML is the reason I'm asking. I've used it in almost every project, but I avoid it for a while before jumping in ... :-P

@lcobucci lcobucci restored the lcobucci:3.0-dev/remove-yaml branch Sep 1, 2016

@lcobucci lcobucci deleted the lcobucci:3.0-dev/remove-yaml branch Sep 1, 2016

@JarJak

This comment has been minimized.

Contributor

JarJak commented Nov 16, 2016

I'd love if someone would still maintain YAML driver.

@Ocramius

This comment has been minimized.

Member

Ocramius commented Nov 16, 2016

We're having major issues in keeping the most important bits maintained and supported (as in helping people out): either somebody builds a repository for YAML support (and maintains that outside of the doctrine organization) or it is not going to happen for now.

@rdohms

This comment has been minimized.

Contributor

rdohms commented May 5, 2017

I represent the repressed people of Yaml and would like to say I stand strong against this carnage of configuration of the yaml kind. YAML IS ALSO TEXT!

Actually, no, kill it with fire.

@mickaelandrieu

This comment has been minimized.

mickaelandrieu commented May 5, 2017

Even if I'm sad (was one of YAML users), I do understand your choices: time and motivation aren't unlimited :)

@Ocramius

This comment has been minimized.

Member

Ocramius commented Feb 7, 2018

Since I'm lazy, here's a longer explanation of why we're dropping the thing: doctrine/DoctrineBundle#776 (comment)

coudenysj added a commit to coudenysj/doctrine2 that referenced this pull request Feb 8, 2018

@zquintana

This comment has been minimized.

zquintana commented Feb 16, 2018

Is Yaml support being dropped? Just visited the documentation today to find it's no longer there for associations.

@afoeder

This comment has been minimized.

Contributor

afoeder commented Feb 16, 2018

yes @zquintana see this PR and the immediate last comment prior to yours.

@Majkl578

This comment has been minimized.

Member

Majkl578 commented Feb 16, 2018

Looks like docs is being build from master, that's not a good idea since master now points to version 3.0-dev with BC breaks and many changes.

@Ocramius can we somehow set the build process to use 2.7 branch instead?

@Ocramius

This comment has been minimized.

Member

Ocramius commented Feb 16, 2018

@Majkl578 I think we should rather have proper versioning of the docs, no? I think @mikeSimonson and @malarzm were working on that.

@iamromeo

This comment has been minimized.

iamromeo commented Feb 23, 2018

@Ocramius I am sorry but what will happen to Symfony projects which used YAML files almost exclusively as the primary driver? Should we not be updating Doctrine anymore? Is there a converter to at least move from YAML to XML and/or Annotations?

@Ocramius

This comment has been minimized.

Member

Ocramius commented Feb 23, 2018

As you may have noticed from the URI of this page, this is not Symfony 😜

There is an orm:convert-mapping command:

https://github.com/doctrine/doctrine2/blob/2.6/lib/Doctrine/ORM/Tools/Console/Command/ConvertMappingCommand.php

@Quix0r

In the end, nice cleanup. I didn't saw any non-yaml related stuff being accidentally removed.

@@ -19,13 +19,12 @@
namespace Doctrine\ORM\Tools;
use Doctrine\Common\ClassLoader;
use Doctrine\Common\Cache\ArrayCache;

This comment has been minimized.

@Quix0r

Quix0r Jul 17, 2018

This has been swapped. All fine?

This comment has been minimized.

@Ocramius

Ocramius Jul 17, 2018

Member

Yeah, simply sorted alphabetically

@@ -1,343 +0,0 @@
<?php

This comment has been minimized.

@Quix0r

Quix0r Jul 17, 2018

Lesser code to take care of. And same for other dropped .php files.

@@ -1,88 +0,0 @@
<?php

This comment has been minimized.

@Quix0r

Quix0r Jul 17, 2018

Lesser code to take care of.

@@ -1,64 +0,0 @@
Doctrine\Tests\Models\CMS\CmsAddress:

This comment has been minimized.

@Quix0r

Quix0r Jul 17, 2018

Lesser code to take care of. And all the other .yml files as well.

@@ -380,13 +378,6 @@ public function testCascadeAllCollapsed()
self::assertEquals(1, count($nodes));
self::assertEquals('cascade-all', $nodes[0]->getName());
} else if ($type == 'yaml') {

This comment has been minimized.

@Quix0r

Quix0r Jul 17, 2018

One static code block lesser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment