Skip to content

Conversation

@weaverryan
Copy link

Hey guys-

This is a rework of PR 510, which had gone through a lot of evolution.

  • New Configuration class for DoctrineMongoDBExtension
  • The cleanup of the class, involving removal of getParameter() calls and the removal of some parameters
  • The removal of the extra "connections" and "document_managers" node in XML. This was redundant and made normalization a huge pain. This breaks BC, but I believe is more correct. (weaverryan@a1ecb92)

I think that the tests overall have decent coverage, but they're still a bit of a mess. Certain tests should test the Configuration merging+normalization, and others should feed raw config values (not loaded from an XML or YAML fixture file) into the actual Configuration Extension class and measure the container afterwards. This distinction is still not well-made, and I believe that's repeated for many DI extension classes. This can easily be solved later in a totally BC way.

Comments welcome - especially from the crowd using this bundle in production. See the full.yml file for a somewhat-full example of the config options.

Thanks!

@weaverryan
Copy link
Author

Much of this will need to be repeated for the DoctrineExtension - so we should get happy with this and then tackle that next.

Also, the XSD was out-of-date when I started and is still very out-dated.

@stof
Copy link

stof commented Feb 19, 2011

What are the DoctrineMongoDBExtension.php.hold and DoctrineMongoDBExtension.php.hottness files ?

I proposed to work on DoctrineExtension during last meeting but I think I would be better to wait for the feedback on this PR to be able to be consistent.

@weaverryan
Copy link
Author

Woops! For all my care, I was still careless. Ill remove those shortly - they were helping me translate my earlier work.

@weaverryan
Copy link
Author

Rebased and extra files are gone - we're ready for feedback.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be removed (this is my very small contribution to this huge patch ;)).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops again - it's gone now - thanks for the close eye.

@fabpot
Copy link
Owner

fabpot commented Feb 21, 2011

Ready to be merged?

@weaverryan
Copy link
Author

Not yet - I'm still working to be sure that there are some OpenSky eyes on this. I'll ping here when that's happened.

@jmikola
Copy link

jmikola commented Feb 21, 2011

This didn't appear to break any of our tests, and the XML config stuff looks good. Keep in mind we have a fairly simple ODM config (one connection and a mapping directory), so your own unit tests likely provide better coverage for that.

Great job!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird as it still requires to handle both syntaxes in the extension (it should be a normalization work) and the syntaxes cannot be used both which is not handled here.

fabpot and others added 29 commits March 8, 2011 15:33
* stof/assets_url_fix:
  [FrameworkBundle] fixed tests to conform to new xsd
  Fixed assets_base_urls configuration
* mvrhov/validatorsTransSlo:
  Draft Slovenian translation
* naderman/watch-debug:
  [AsseticBundle] Throw exception if --watch is used without --debug.
… the parameter %wildcard% syntax instead. This is best-practice.
… array without the wrapping array.

Also rewrote this so that it's more clear.
…ad() method discussing the available options.
…s" and "document_managers" wrapper in XML. This was inconsistent and made normalization unnecesarily difficult.

Previously:

    <connections>
        <connection ...>
            <!-- .. -->
        </connection>
        <connection ...>
            <!-- .. -->
        </connection>
    </connections>

Now:

    <connection ...>
        <!-- .. -->
    </connection>
    <connection ...>
        <!-- .. -->
    </connection>

I believe this is more semantically correct, and it removes code that had to worry about this extra format and normalize it.
…ration class and updated a few default values.
…ache_driver" parameter.

This information was set entirely in the DI extension options and then added as a parameter only to be later consumed by the DI extension class.

In other words, this should never have been a parameter, or at least it's never been used and treated like a parameter.
…rs(), which is truer to its name.

Also mae this function unset the options after their used. This prevents those values from being available later inside the options array and as parameters.
…r, which was never used as a parameter. Now cleaning up connections creation in the DI extension class.
…ded in the Extension and removing two unneeded DIC parameters.
…mall things to match pull request 99 for the Doctrine Configuration class
@weaverryan
Copy link
Author

Closed and moved to symfony/symfony so that it could be rebased to the current code: symfony#209

This pull request was closed.
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.