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

Add namespace Env #7

Merged
merged 1 commit into from
May 28, 2020
Merged

Add namespace Env #7

merged 1 commit into from
May 28, 2020

Conversation

tangrufus
Copy link
Contributor

@tangrufus tangrufus commented Apr 29, 2020

Namespace Env is added to composer.json since the first commit. However, the Env class lives under the global namespace.

env/composer.json

Lines 24 to 27 in 4ab45ce

"autoload": {
"psr-0": {
"Env": "src/"
}

Goal: To avoid issues like roots/bedrock#515 and roots/bedrock#516

Note: This is a breaking change.


With the namespace, we can autoload env_function.php with composer so that we dont need Env::init()


Using the namespace OscarOtero\Env might be a better choice - less chance colliding with other packages.

@oscarotero
Copy link
Owner

The reason for not using namespaces in this library is to bring support for PHP 5.2 (that was supported by WordPress sometime. But I'm agree that this is completely outdated and I'm ok with adding the namespace Env\Env.

What I'm not fully convinced is about namespacing the function. The idea of having a function was precisely for convenience and to do not need to add a use Env in every file you want to get an environment variable. And this is why this function is not loaded automatically by composer, but only if the user want to use it, to avoid polluting the global scope with no need. If the user need to do a use function Env\env to use the function in every file, I don't see any advantage because she can use Env\Env::get() directly.

What do you think?

@Silic0nS0ldier
Copy link

If everything is to be namespaced, keeping the env function seems a little redundant. The point is convenience, but with the proposed change either a use statement is needed or \Env\env('...');. On top of this init still needs to be called (since PHP currently doesn't support autoloading namespaced functions).

Re OscarOtero\Env vs. just Env for the namespace, I'm in favour of the longer option. Collisons (although thankfully rare) do happen and tend to present a major challenge.

@tangrufus tangrufus force-pushed the namespace branch 3 times, most recently from a1b34b8 to dab7b5b Compare May 26, 2020 01:48
@tangrufus tangrufus changed the title Add namespace Env Add namespace OscarOtero\Env May 26, 2020
@tangrufus
Copy link
Contributor Author

  • Change namespace to OscarOtero\Env (I agree with @Silic0nS0ldier )
  • Remove Env::init()
  • Remove env()
  • Bump minimum PHP requirement to v5.6
  • Update TravisCI build matrix to build PHP 5.6~7.4 and nightly
  • Install phpunit via composer
  • Add $ composer test

@Silic0nS0ldier
Copy link

@tangrufus You meant OscarOtero not OscaOtero right (in the code)? Typo I'm guessing.

@tangrufus tangrufus force-pushed the namespace branch 2 times, most recently from edfc958 to 833cba0 Compare May 26, 2020 02:12
@tangrufus
Copy link
Contributor Author

You are correct. I need ☕

@oscarotero
Copy link
Owner

Thanks for this work, but I'm not fully confortable using my name as a namespace of a library. I use it as the vendor of the composer package, but I think the code should reflect the library functionality, not the author.

So, I'd rather use Env instead OscarOtero.

@tangrufus
Copy link
Contributor Author

tangrufus commented May 27, 2020

Final allure, I believe using VendorName\PackageName as the namespace is the best practice:

  • PSR-0

A fully-qualified namespace and class must have the following structure \<Vendor Name>\(<Namespace>\)*<Class Name>

-- https://github.com/php-fig/fig-standards/blob/032aa6dc3fb4c66072128f1d4aecc135c623e9ca/accepted/PSR-0.md#mandatory

  • PSR-4

The fully qualified class name MUST have a top-level namespace name, also known as a "vendor namespace".

-- https://github.com/php-fig/fig-standards/blob/032aa6dc3fb4c66072128f1d4aecc135c623e9ca/accepted/PSR-4-autoloader.md#2-specification

  • PSR-12

PSR-12 uses namespace Vendor\Package; in the examples.

See: https://github.com/php-fig/fig-standards/blob/aea388d8d692491791c49b03a51314f7e7da294d/accepted/PSR-12-extended-coding-style-guide.md

  • Symfony

Prefix the name with the concatenation of the vendor (and optionally the category namespaces);

-- https://symfony.com/doc/4.1/bundles/best_practices.html#bundle-name

  • Laravel

Laravel packages use Illuminate as vendor namespace.

  • illuminate/pagination --> namespace Illuminate\Pagination;
  • illuminate/view --> namespace Illuminate\View;

Let me know if you are not convinced with namespace OscarOtero\Env;. I will push new commits to change it. @oscarotero

@oscarotero
Copy link
Owner

@tangrufus I agree in using a namespace but what I'm saying is that I don't like OscarOtero as namespace :).

Some examples:

https://github.com/oscarotero/middleland is registered in packagist as oscarotero/middleland but the root namespace is Middleland (because it's the package "brand").

https://github.com/oscarotero/typofixer has the same approach.

All these packages are independent, there's no a OscarOtero framework or organization with a suite of packages. Using OscarOtero as a namespace requires to organize all my packages in sub-namespaces (OscarOtero\Middleland, OscarOtero\Typofixer, OscarOtero\Env etc). Maybe it was an error registering all these packages with oscarotero vendor in packagist in the first place, and something like middleland/middleland, typofixer/typofixer and env/env could be better (other packages of mine like embed/embed or gettext/gettext use that approach).

So I'd suggest to use Env as the namespace, the class would be Env\Env and the function Env\env. It's shorter and easier to remember.

And if you don't want to have a namespace different from vendor name, I can consider change the vendor name to env and publish it as env/env. (Seems like this vendor name is not being used in packagist at this moment).

What do you think?

@tangrufus tangrufus changed the title Add namespace OscarOtero\Env Add namespace Env May 27, 2020
@tangrufus
Copy link
Contributor Author

Understood. Changed namespace to Env.

@oscarotero
Copy link
Owner

Great, thank you!
Just one thing: you have removed Env\env function, that's correct? I did understand we're going to keep it

- Add namespace `Env`
- Remove `Env::init()`
- Remove `env()`
- Bump minimum PHP requirement to v5.6
- Update TravisCI build matrix to build PHP 5.6~7.4 and nightly
- Install phpunit via composer
- Add `$ composer test`
@tangrufus
Copy link
Contributor Author

Added env() back.

@oscarotero oscarotero merged commit a2f2a57 into oscarotero:master May 28, 2020
@oscarotero
Copy link
Owner

Thanks! 👍

@tangrufus
Copy link
Contributor Author

Thanks all!

Any other planned feature for v2.0.0 tag?

@oscarotero
Copy link
Owner

I think we can remove support for php5 (or even php 7.0).
Bedrock requires php 7.1 and we can take advantage of advanced typing.

@oscarotero
Copy link
Owner

Ok, new version 2.0.0 released.

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.

3 participants