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

[5.8] Handle database urls for database connections. #28308

Merged
merged 7 commits into from Apr 30, 2019

Conversation

@mathieutu
Copy link
Contributor

commented Apr 23, 2019

Following #28096

I've directly integrated it in the connection process, so it's fully transparent for the user, and without any breaking change (see the tests about it).

Usages

[
	'default' => env('DB_CONNECTION', 'mysql'),
	'connections' => [
		'mysql' => [
			'url' => env('DATABASE_URL'), // will override other parameters
            'driver' => 'mysql',
            'host' => env('DB_HOST', '127.0.0.1'),
            'port' => env('DB_PORT', '3306'),
            'database' => env('DB_DATABASE', 'forge'),
            'username' => env('DB_USERNAME', 'forge'),
            'password' => env('DB_PASSWORD', ''),
            'unix_socket' => env('DB_SOCKET', ''),
            'charset' => 'utf8mb4',
            'collation' => 'utf8mb4_unicode_ci',
			// ...
		],

		// Or directly: 
		'url' => env('DATABASE_URL')
// ...

I've covered many cases and examples in tests, so I let you dig into them, and give me back your thoughts about it.

I'm also using it in a real project, and for now it works like a charm.

Thanks.
Matt'

mathieutu added 2 commits Apr 23, 2019

@mathieutu mathieutu changed the title [5.8] Handle database urls for database connection. [5.8] Handle database urls for database connections. Apr 23, 2019

mathieutu added 3 commits Apr 23, 2019
@driesvints
Copy link
Member

left a comment

Just some conventions you need to be aware of.

src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
src/Illuminate/Database/DatabaseManager.php Outdated Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
src/Illuminate/Database/UrlParser.php Outdated Show resolved Hide resolved
tests/Database/DatabaseConnectionFactoryTest.php Outdated Show resolved Hide resolved
Apply @driesvints suggestions from code review.
Co-Authored-By: mathieutu <oss@mathieutu.dev>
@mathieutu
Copy link
Contributor Author

left a comment

@driesvints I applied your suggestions and will add all the docblocks in a future commit.

BUT I really don't understand why you want to replace all the typehints by phpdoc. I understand that typehints can create issues in some cases (magic methods, proxy, etc.) but not here.

When methods can only handle strings and return strings, why not using language features, instead of adding comments to try to reproduce this native behavior?

This is a real question, because I'm wondering what I'm missing here.

Thanks.

src/Illuminate/Database/UrlParser.php Show resolved Hide resolved
tests/Database/DatabaseConnectionFactoryTest.php Outdated Show resolved Hide resolved
@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I really don't understand why you want to replace all the typehints by phpdoc.

@mathieutu because it's the current convention of the framework and its libraries. When a convention is in place it's best to stick to it.

On a personal note I'm also in favor of introducing types but it would need to be introduced in a new major release somewhere in the future: laravel/ideas#1409

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Please also make UrlParser::$driverAliases protected and replace self:: with static::. This allows users to override any functionality.

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@driesvints ok, understood the point, thanks (event if there is already a lot of typehints in framework, like all the array you left). Is that convention written somewhere?

Another thing: You have converted all private methods and properties to protected, so the class can be extended easily.

I agree with that.

But as you also replaced UrlParser instantiation from container to direct new UrlParser in code, devs finally can't replace it by the class they newly created.

If it stays instanced by the container, it will be easy for devs to overwrite it in a service provider. 😉

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@driesvints ok, understood the point, thanks (event if there is already a lot of typehints in framework, like all the array you left). Is that convention written somewhere?

No it's not. It's currently just general know-how from looking at the code already in place. PRs to the docs to improve this are appreciated.

If it stays instanced by the container, it will be easy for devs to overwrite it in a service provider. 😉

I don't consider this to be a reason to resolve it through the container. There are other ways to overwrite this. What you could do is pass it as a constructor argument and then use the container outside the actual object to resolve it.

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

What you could do is pass it as a constructor argument and then use the container outside the actual object to resolve it.

Can totally do that if you prefer, but for me it just moves the problem to higher levels (actually two: DatabaseManager is instancied in src/Illuminate/Database/Capsule/Manager.php:63 and src/Illuminate/Database/DatabaseServiceProvider.php:62).

The container was already passed and used directly in the class, it's why I did that way.

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

The container was already passed and used directly in the class, it's why I did that way.

Yeah, you're right. That's a bit unfortunate but I believe that we should refrain from adding new implementations of which use that dependency. That's why I think another constructor dependency is the way to go. At both places where the manager is instantiated you can use the container to resolve the dependency. This would have to go to 5.9 then though because it's a breaking change for anyone using the manager separately.

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@driesvints Made the changes, and added all the docblock.

I let the container part to someone else, for the day there will be a need.

And, just to finish on the "convention" part, I let you see what the boss was doing 2 years ago: https://github.com/laravel/framework/blame/56d11705d817123715a0b64d6daf92963eaa26f0/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php#L55 😛

(just a bit of :trollface:, only to say that in my opinion we should not loose time on things like that, and just take advantage of what the language has to offer. Good improvements are coming quite quickly to PHP, and it would be too bad that an accumulation of debt let us years back.)

Thanks anyway for the time you spent helping me with this PR!

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I let the container part to someone else, for the day there will be a need.

No problem.

And, just to finish on the "convention" part, I let you see what the boss was doing 2 years ago:

Hah! I do think that's a real exception though.

Thanks for your work!

@taylorotwell taylorotwell merged commit 1b568d0 into laravel:5.8 Apr 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@djtarazona

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Thanks for the great work @mathieutu! Does this only apply to database connections? It would be great to have this for Redis connections as well (Heroku Redis uses this URL format).

@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

❤️

I can get rid of my hack-y overrides at the top of my config/database.php files in applications I deploy to Heroku now 😄

<?php

if (getenv('DATABASE_URL')) {
    // Parse URL and set individual config keys
}
@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@djtarazona Hi, actually everything is made to be used with redis too.

It may still needs a call to do in the proper place, I have to check how Redis is handled in Laravel (I'm a bit rusted about that).

If needed and if you want to open a PR, I can help you with that.

We also need to document that, and with my poor english some help will be much needed!

@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@mathielo What version of Laravel is this available in?

@driesvints

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@martinbean v5.8.15 at least

@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@driesvints Cool. Thanks!

@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@mathielo Do you have an example of how this is supposed to be used?

I’ve just updated an application to Laravel 5.8.15 (confirmed by running php artisan --version) and removed the DB_* variables from my .env and replaced them with this line:

DATABASE_URL=postgres://homestead:secret@127.0.0.1:5432/tickets

However, my application seems to try and connect to the default connection (mysql) using the default values in the environment variables (i.e. forge for the username, etc).

@dwightwatson

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

You'll need to update your database.php and replace all the separated keys with a single url key:

'pgsql' => [
    'url' => env('DATABASE_URL'),
    'charset' => 'utf8',
    'prefix' => '',
    'prefix_indexes' => true,
    'schema' => 'public',
    'sslmode' => 'prefer',
],

Note that you can also remove the driver key as it's inferred from the URL.

@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Never mind, worked it out for myself! I needed to add a url key with the environment variable to the configuration, i.e.

return [
    'connections' => [
        'pgsql' => [
+           'url' => env('DATABASE_URL', null),
            'driver' => 'pgsql',
            'host' => env('DB_HOST', '127.0.0.1'),
            'port' => env('DB_PORT', '5432'),
            'database' => env('DB_DATABASE', 'forge'),
            'username' => env('DB_USERNAME', 'forge'),
            'password' => env('DB_PASSWORD', ''),
            'charset' => 'utf8',
            'prefix' => '',
            'prefix_indexes' => true,
            'schema' => 'public',
            'sslmode' => 'prefer',
        ],
    ],
];
@mathielo

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@martinbean no, I do not. 😂 You probably want to ping @mathieutu. Cheers!

@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@mathielo Sorry, picked wrong person from the mention auto-complete 😂

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

It's exactly that!
(And I definitely need to add it to Laravel core and document it.)

return [
    'connections' => [
        'pgsql' => [
+           'url' => env('DATABASE_URL', null),
            'driver' => 'pgsql',
            'host' => env('DB_HOST', '127.0.0.1'),
            'port' => env('DB_PORT', '5432'),
            'database' => env('DB_DATABASE', 'forge'),
            'username' => env('DB_USERNAME', 'forge'),
            'password' => env('DB_PASSWORD', ''),
            'charset' => 'utf8',
            'prefix' => '',
            'prefix_indexes' => true,
            'schema' => 'public',
            'sslmode' => 'prefer',
        ],
    ],
];
@martinbean

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@mathieutu Cool. Is this working for Redis configuration, too?

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@martinbean I let you try and let me know? Will doing the corresponding PR, depending of your answer. 😉

@hmazter

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@mathieutu As far as I can see the url is not implemented for redis.
I see no usage of the ConfigurationUrlParser in the RedisManager and when I'm just testing to pass a url property instead of host, port and pass to redis it is not used.

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@hmazter

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@mathieutu Thanks!

@tillkruss

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@mathieutu: Does this work with Redis now?

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@tillkruss

Ok thanks. I'll try to do a PR for that in the week-end.

I sware I'll take a look this week-end 😉

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Hey guys!
I'm not personally a redis user, so I've taken a look at the Predis documentation and... I can confirm that the url is already handled by the lib for a while!

https://github.com/nrk/predis/wiki/Connection-Parameters

However, an array is typed for the $config variable in \Illuminate\Redis\Connectors\PredisConnector::connect.

Passing it as an array like that works well:

'redis' => [
    'client' => 'predis',
    'default' => [
        'redis://h:password@host:12499',
    ],
],

BUT:

By default, when no specific client options are set and an array of connection parameters is passed to the client's constructor, Predis configures itself to work in clustering mode
https://github.com/nrk/predis/#user-content-cluster

So in conclusion, in my opinion, we don't even need any preprocessing on our side (we totally could, but don't need), we just have to remove the type hint of the connector (and I'd be glad to do the PR 😉).

I let you give me your feedback about that, as you will have much more experience with this topic than me!

Thoughts?

@hmazter

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I can confirm that this is a working config for the redis database when using the predis client.

    'redis' => [
        'client' => 'predis',
        'default' => [
            env('REDIS_URL', 'redis://127.0.0.1:6379') . '?database=0',
        ],
        'session' => [
            env('REDIS_URL', 'redis://127.0.0.1:6379') . '?database=1',
        ],
    ],

BUT, it does not seem to work when using phpredis instead of predis.

@tillkruss

This comment has been minimized.

Copy link
Member

commented May 19, 2019

It doesn't work with PhpRedis:

Undefined index: host
[
    'redis' => [

        'client' => env('REDIS_CLIENT', 'phpredis'),

        'options' => [
            'cluster' => env('REDIS_CLUSTER', 'predis'),
            'prefix' => env('REDIS_PREFIX'),
        ],

        'default' => [
            env('REDIS_URL', 'redis://127.0.0.1:6379') . '?database=' . env('REDIS_DB', 0),
        ],

        'queues' => [
            env('REDIS_URL', 'redis://127.0.0.1:6379') . '?database=' . env('REDIS_QUEUES_DB', 1),
        ],

        'sessions' => [
            env('REDIS_URL', 'redis://127.0.0.1:6379') . '?database=' . env('REDIS_SESSIONS_DB', 2),
        ],

        'cache' => [
            env('REDIS_URL', 'redis://127.0.0.1:6379') . '?database=' . env('REDIS_CACHE_DB', 3),
        ],

    ],
]
@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Ok, so let's go, I'll do exactly the same than for databases, to be able to configure easily with an url key (and so add database in its dedicated key aside).

I'll try to do that tomorrow (could even be released during the day if my students work a bit autonomously! 😇)

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Here we go!
Later than expected, but this one is for Redis:
#28612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.