adds a new output format #598

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Member

schmittjoh commented Mar 3, 2013

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2329

@Ocramius Ocramius commented on the diff Mar 3, 2013

lib/Doctrine/ORM/Tools/Console/Command/RunDqlCommand.php
@@ -25,6 +25,7 @@
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Doctrine\Common\Util\Debug;
+use JMS\Serializer\SerializerBuilder;
@schmittjoh

schmittjoh Mar 3, 2013

Member

This cannot be removed.

As a side note, this will not actually require this class to be present, i.e. it does not add a hard dependency. The class is only required if someone actually uses the new output format.

@Ocramius Ocramius commented on the diff Mar 3, 2013

lib/Doctrine/ORM/Tools/Console/Command/RunDqlCommand.php
@@ -118,6 +124,22 @@ protected function execute(InputInterface $input, OutputInterface $output)
$resultSet = $query->execute(array(), constant($hydrationMode));
- Debug::dump($resultSet, $input->getOption('depth'));
+ switch ($input->getOption('format')) {
+ case 'doctrine-debug':
+ ob_start();
+ Debug::dump($resultSet, $input->getOption('depth'));
+ $message = ob_get_clean();
+
+ $output->write($message);
+ break;
+
+ case 'jms-serializer-json':
+ $serializer = SerializerBuilder::create()->build();
@Ocramius

Ocramius Mar 3, 2013

Owner

Hmm... Not sure this would work in the ORM :| What about wrapping the command in the bundle?

@schmittjoh

schmittjoh Mar 3, 2013

Member

Could you elaborate?

For now, I've had to copy/pasted the entire command.

@stof

stof Mar 3, 2013

Member

@Ocramius It is not tied to the bundle at all. It simply require that you have the jms/serializer library installed in your project to be able to use this format

@Ocramius

Ocramius Mar 3, 2013

Owner

@stof then let's add it to the composer suggests :) (yes, I knew it wasn't the bundle)

@guilhermeblanco

guilhermeblanco Mar 3, 2013

Owner

I'm -1 on this.
JMS Serializer creates a cyclic dependency to Doctrine which is a no go for us.

@schmittjoh

schmittjoh Mar 3, 2013

Member

First, I don't see any problems with using the serializer here in practice. Maybe you can share where you think it would cause problems?

Apart from that, I'm not bound to using "my" library here, if you can suggest other ways how we can make the currently unstable format more robust, I'm open to that as well.

@Ocramius

Ocramius Mar 3, 2013

Owner

@schmittjoh what if we just provide (not necessarily in ORM) a return value and that return value is used by a new SomeFormatRunDqlCommand() that can produce an output based on a format flag instead?

This command here has mainly been used for debugging. While the idea is cool, the serializer (or the serializer bundle) seems more appropriate for this, no?

They could also just wrap existing commands:

  • new FormatOutputCommand(new RunSQLCommand());
  • new FormatOutputCommand(new RunDQLCommand());
  • new FormatOutputCommand(new RunSOAPSomethingCommand());
@guilhermeblanco

guilhermeblanco Mar 3, 2013

Owner

@schmittjoh the problem is that your bundle creates a cyclic dependency between Doctrine <-> Serializer. One could not live without the other and vice versa. While we can try to reduce this coupling to a suggest level, there's still a soft dependency between them.
My idea would be linear to what @Ocramius suggests. We could create a simplified version of your Serializer that does not have a dependency of ORM or DBAL and consume it inside them.
It's not an advocate about owners of library. It's about resolving dependencies between packages.

@schmittjoh

schmittjoh Mar 3, 2013

Member

As for use cases, I'm using this in tests to introspect the database.

With regard to wrapper commands, I'm not a fan of said practice for several reasons (boiler plate code, loading/dependency issues, and more), so that is not an option for me.

@stof

stof Mar 4, 2013

Member

@guilhermeblanco the ORM would only have an optional dependency to the serializer. And the serializer only has an optional integration for Doctrine. I don't think it is a big problem here.

@beberlei

beberlei Mar 4, 2013

Owner

Its not a requirement of the output here to be robust - its used for debugging, and adding a hard dependency is not an option as @guilhermeblanco already said.

If you introduce an interface + debug implementation (maybe even reuse from the DBAL command) and then implement the serializer dumper in your code then I am fine, but we shouldn't add this as a suggest dependency just because it is used in a debugging command that is used by hardly anyone.

@schmittjoh

schmittjoh Mar 4, 2013

Member

I agree, the scope of this is so minimal that I haven't added the suggest entry. Anyway, I've already duplicated this command in my TestBundle, so let's not waste time on this. I'll close it.

schmittjoh closed this Mar 4, 2013

schmittjoh deleted the schmittjoh:newOutputFormat branch Mar 4, 2013

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