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

Support for 'url' parameter in config #352

Closed
wants to merge 3 commits into from

Conversation

dzuelke
Copy link
Contributor

@dzuelke dzuelke commented Dec 7, 2014

Supported since Doctrine 2.5.0 :)

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 7, 2014

Totally forgot about this part in addition to support for "url" in DBAL, @beberlei and @stof.

@xabbuh
Copy link
Member

xabbuh commented Dec 7, 2014

@dzuelke I guess the XSD file has to be updated as well.

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 7, 2014

Good point @xabbuh, updated (and some docs, too)

URL will override explicitly set parameters. An example database URL
would be "mysql://snoopy:redbaron@localhost/baseball", and any explicitly
set driver, user, password and dbname parameter would be overridden by
this URL. See the Doctrine `DBAL documentation`_ for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Can the referenced URL somehow be improved? You will now reach the front page of the Doctrine DBAL documentation and you will then have to search for the right page explaining the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for now, the Doctrine DBAL documentation on their website hasn't been updated in ages. Paging @beberlei.

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 8, 2014

Paging @stof / @kimhemsoe / @guilhermeblanco - can we get this merged and released please? Otherwise that new DBAL feature can't be used.

@@ -316,6 +325,7 @@ can configure. The following block shows all possible configuration keys:
<doctrine:config>
<doctrine:dbal
name="default"
url="mysql://user:secret@localhost:1234/otherdatabase" <!-- this would override the values below -->
Copy link
Member

Choose a reason for hiding this comment

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

Is the inline comment valid here? Never saw one in the middle of a tag, so I may be wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but the same is used further down, so I figured it didn't matter.

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 8, 2014

@beberlei The tests are failing because doctrine/orm 2.4's SchemaValidator::validateClass() does

foreach ($class->reflClass->getProperties(\ReflectionProperty::IS_PUBLIC) as $publicAttr) {

but $class->reflClass is NULL.

This is unrelated to my PR; the tests fail on master etc as well.

Seems to be a version incompatibility thing; it fetches lots of dev dependencies due to minimum-stability in composer.json.

What to do?

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 8, 2014

Changing the version requirement for doctrine/orm to dev-master and removing minimum-stability in composer.json fixes it. Changing only the version but leaving minimum-stability causes a lot of deprecation notices.

Yay version hell.

@Ocramius
Copy link
Member

I would just bump the version requirement to 2.5, but installation will fail until the ORM is actually released

@stof
Copy link
Member

stof commented Dec 12, 2014

I would just bump the version requirement to 2.5, but installation will fail until the ORM is actually released

Please don't bump the requirement to 2.5 if it is not released yet. I don't want to have to maintain multiple versions in parallel because of that

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 12, 2014

Yeah, looks like an ORM 2.5 is close though. Just a few issues left, and some of them look like they'll be moved to 3.0. Poking @beberlei again :p

@stof
Copy link
Member

stof commented Dec 12, 2014

Btw, if the SchemaValidator is broken on Doctrine ORM 2.4, please report an issue on Doctrine ORM itself, as it should be fixed.

@Ocramius
Copy link
Member

@dzuelke I'll have to tag the ORM in few days: requires a full week of work though.

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 12, 2014

I don't think it's broken, @stof. It's just an incompatibility between ORM 2.4 and something else. When you "composer install" on DoctrineBundle itself, the "minimum-stability": "dev" takes effect (can that be removed?) and that messes stuff up I think.

@dzuelke
Copy link
Contributor Author

dzuelke commented Dec 12, 2014

I have really no idea how this test ever passed. Nothing related to it seems to have changed in the bundle or in ORM.

These:

    const FIRST_ENTITY = 'TestBundle\Test\Entity\Test1';
    const SECOND_ENTITY = 'TestBundle\Test\Entity\Test2';

in DoctrineDataCollectorTest don't exist, so of course $class->reflClass in ORM's SchemaValidator::validateClass() is null.

Fixing that foreach() to not run if $class->reflClass is null does the job, but then the test fails:

  1. Doctrine\Bundle\DoctrineBundle\Tests\DataCollector\DoctrineDataCollectorTest::testCollectEntities
    Expectation failed for method name is equal to string:isSecondLevelCacheEnabled when invoked 1 time(s).
    Method was expected to be called 1 times, actually called 0 times.

There is some big arse version conflict thing going on that I can't figure out. I mean, the tests are failing even if you just run them on the bundle's master, not just this pull request.

Something, somewhere, changed and that causes the tests to fail.

@dzuelke
Copy link
Contributor Author

dzuelke commented Jan 8, 2015

bump?

/cc @deeky666 @Ocramius @stof

@Ocramius
Copy link
Member

Ocramius commented Jan 8, 2015

Assigning @kimhemsoe for review/merge

@dzuelke
Copy link
Contributor Author

dzuelke commented Jan 8, 2015

Well this works and is pretty trivial, @Ocramius, but the real issue is that tests are still failing due to unrelated version conflicts, even on master, and there's no solution and I haven't figured it out yet (I saw you closed the doctrine/orm#1237 PR that attempted to fix this). Should I spin that off into its own bug report?

@deeky666
Copy link
Member

deeky666 commented Jan 8, 2015

@dzuelke those unrelated test failures are fine for now. I already opened a PR #364 that deals with this issue, so no further action necessary here atm.
Otherwise looking good so far, despite it is missing some trivial tests.

@dzuelke
Copy link
Contributor Author

dzuelke commented Jan 9, 2015

Should I put the tests into DoctrineExtensionTest or somewhere else, @deeky666 ?

@stof
Copy link
Member

stof commented Jan 9, 2015

I merged the Pr fixing the testsuite. Can you rebase your branch to rerun the testsuite with the fix ?

@dzuelke
Copy link
Contributor Author

dzuelke commented Jan 9, 2015

Done. Looks like all is well!

@stof
Copy link
Member

stof commented Jan 9, 2015

Thanks @dzuelke.

@stof stof closed this in a7be436 Jan 9, 2015
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.

None yet

6 participants