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

Fatal: hasOption() on null #1181

Merged
merged 1 commit into from
Oct 11, 2017
Merged

Fatal: hasOption() on null #1181

merged 1 commit into from
Oct 11, 2017

Conversation

vaeryn-uk
Copy link
Contributor

@vaeryn-uk vaeryn-uk commented Sep 12, 2017

Hi. Not sure if I'm missing something, but we've encountered an issue when instantiating a migration environment.

Sample code:

<?php

require __DIR__ . '/vendor/autoload.php';

$manager = new \Phinx\Migration\Manager(
    \Phinx\Config\Config::fromPhp(__DIR__ . '/test-config.php'),
    new \Symfony\Component\Console\Input\ArrayInput([]),
    new \Symfony\Component\Console\Output\NullOutput()
);

class TestSeed extends \Phinx\Seed\AbstractSeed { }

$manager->executeSeed('development', new TestSeed());

Where our test configuration is:

<?php

return [
    'environments' => [
        'development' => [
            'name' => 'devdb',
            'connection' => new PDO('sqlite::memory:'),
        ]
    ]
];

We encounter this error:

Fatal error: Uncaught Error: Call to a member function hasOption() on null in phinx/src/Phinx/Db/Adapter/AbstractAdapter.php on line 248

Error: Call to a member function hasOption() on null in phinx/src/Phinx/Db/Adapter/AbstractAdapter.php on line 248

Call Stack:
    0.0002     371912   1. {main}() phinx/pop.php:0
    0.0030     996640   2. Phinx\Migration\Manager->executeSeed() phinx/pop.php:13
    0.0032    1035552   3. Phinx\Migration\Manager\Environment->executeSeed() phinx/src/Phinx/Migration/Manager.php:396
    0.0032    1035552   4. Phinx\Migration\Manager\Environment->getAdapter() phinx/src/Phinx/Migration/Manager/Environment.php:148
    0.0033    1049152   5. Phinx\Db\Adapter\AdapterFactory->getAdapter() phinx/src/Phinx/Migration/Manager/Environment.php:344
    0.0044    1349128   6. Phinx\Db\Adapter\AbstractAdapter->__construct() phinx/src/Phinx/Db/Adapter/AdapterFactory.php:131
    0.0044    1349128   7. Phinx\Db\Adapter\PdoAdapter->setOptions() phinx/src/Phinx/Db/Adapter/AbstractAdapter.php:71
    0.0044    1349128   8. Phinx\Db\Adapter\PdoAdapter->setConnection() phinx/src/Phinx/Db/Adapter/PdoAdapter.php:56
    0.0045    1349128   9. Phinx\Db\Adapter\AbstractAdapter->createSchemaTable() phinx/src/Phinx/Db/Adapter/PdoAdapter.php:74
    0.0050    1452504  10. Phinx\Db\Table->save() phinx/src/Phinx/Db/Adapter/AbstractAdapter.php:218
    0.0050    1452504  11. Phinx\Db\Table->create() phinx/src/Phinx/Db/Table.php:697
    0.0050    1452504  12. Phinx\Db\Adapter\SQLiteAdapter->createTable() phinx/src/Phinx/Db/Table.php:609
    0.0051    1452760  13. Phinx\Db\Adapter\PdoAdapter->execute() phinx/src/Phinx/Db/Adapter/SQLiteAdapter.php:228
    0.0051    1452760  14. Phinx\Db\Adapter\AbstractAdapter->isDryRunEnabled() phinx/src/Phinx/Db/Adapter/PdoAdapter.php:126

Our script runs an empty seed to prove the problem, but I suspect this would happen with any DB operation against the manager. I think the problem is around initialising a connection, which is too early for an environment to be aware of a console input.

We've worked around this in our project by reverting to v0.8.1, but I'm posting this quick-fix PR in case it is any use.

Thanks! :)

@vaeryn-uk vaeryn-uk force-pushed the dry-run-fix branch 3 times, most recently from bf8abed to ed672cf Compare September 12, 2017 12:23
@JayPHP
Copy link
Member

JayPHP commented Sep 12, 2017

@Ehimen Thanks for this, looks good :)

Not sure about the changes to the PDO Adapter test though? Did you mean to commit that?

@vaeryn-uk
Copy link
Contributor Author

vaeryn-uk commented Sep 12, 2017

I did, yeah. They're a bit weird to cater for older PHPUnit (following required PHP versions in the CI build), but that does cover this case. Without the fix, the test would fail with the same hasOption() on null bug.

@Veridis
Copy link

Veridis commented Sep 12, 2017

Is there a workaround until this PR is accepted ?

@JayPHP
Copy link
Member

JayPHP commented Sep 13, 2017

@Veridis Ok. You've also renamed the test, can you undo that?

@vaeryn-uk
Copy link
Contributor Author

Sorry @JayPHP. Not sure what you mean?

@codecov-io
Copy link

codecov-io commented Sep 18, 2017

Codecov Report

Merging #1181 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
- Coverage   72.13%   72.01%   -0.13%     
==========================================
  Files          35       35              
  Lines        5592     5592              
==========================================
- Hits         4034     4027       -7     
- Misses       1558     1565       +7
Impacted Files Coverage Δ
src/Phinx/Db/Adapter/AbstractAdapter.php 80.59% <100%> (ø) ⬆️
src/Phinx/Db/Adapter/PdoAdapter.php 87.36% <0%> (-3.85%) ⬇️

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 6a6ebc7...4c26aeb. Read the comment docs.

@ahfeel
Copy link

ahfeel commented Sep 19, 2017

+1, this is actually a critical issue. It's impossible to boostrap a new project with a PDO adapter to a database that doesn't have the phinxlog table already created.

@bobmagicii
Copy link

bobmagicii commented Sep 26, 2017

oh man, so glad i am not going insane. been trying to bootstrap for like two hours now. can confirm the above fix works.

@vaeryn-uk
Copy link
Contributor Author

@JayPHP / @lorenzo , is this something we can get merged? Happy to make any changes required. Thanks :)

@dereuromark
Copy link
Member

Looks like a good change!

@burzum
Copy link

burzum commented Oct 8, 2017

@lorenzo let's merge it.

@JayPHP JayPHP merged commit ba9c0b0 into cakephp:master Oct 11, 2017
@JayPHP
Copy link
Member

JayPHP commented Oct 11, 2017

Thanks @Ehimen !

ndm2 pushed a commit to ndm2/phinx that referenced this pull request Nov 3, 2017
Unit test files must end with the `Test` suffix, otherwise they're not
being run.

refs cakephp#1181
lorenzo pushed a commit that referenced this pull request Nov 3, 2017
Unit test files must end with the `Test` suffix, otherwise they're not
being run.

refs #1181
K-Phoen added a commit to K-Phoen/regis that referenced this pull request Nov 26, 2017
norkus256 pushed a commit to norkus256/phinx that referenced this pull request Dec 17, 2017
Unit test files must end with the `Test` suffix, otherwise they're not
being run.

refs cakephp#1181
@hazolsky
Copy link

Hi there! Any ETA on when this will be released?

@vaeryn-uk vaeryn-uk deleted the dry-run-fix branch December 22, 2017 11:09
@vaeryn-uk
Copy link
Contributor Author

I've just realised the test file rename. I didn't intend this; must have been my IDE. So despite the coverage dropping and @JayPHP explicitly mentioning this, I still missed it. Apologies all.

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.

9 participants