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

Config helper #1073

Merged
merged 5 commits into from Jun 28, 2018
Merged

Config helper #1073

merged 5 commits into from Jun 28, 2018

Conversation

puschie286
Copy link
Contributor

config helper implementation for #1071

Signed-off-by: Christoph Potas christoph286@googlemail.com

+ create helper function for simplest access

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
~ remove namespace from class name

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
@lonnieezell
Copy link
Member

Looks good. Please add docs and some tests for the class please, and then we are good to go.

~ fix incorrect non-class files handling
+ add tests for all cases

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
Signed-off-by: Christoph Potas <christoph286@googlemail.com>
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Please provide an example in the docs about using the config helper with a full namespace, and then I'll be happy to merge!

Signed-off-by: Christoph Potas <christoph286@googlemail.com>
@lonnieezell lonnieezell merged commit e0501ac into codeigniter4:develop Jun 28, 2018
@puschie286 puschie286 deleted the ConfigHelper branch June 28, 2018 14:41
@jim-parry
Copy link
Contributor

Something is broken in the travis-ci unit tests.
This PR actually did not pass the tests ... I get the following when I run it locally:

  1. CodeIgniter\Config\ConfigTest::testCreateSingleInstance
    Failed asserting that null is an instance of class "Config\Email".

/pub7/htdocs/CodeIgniter4/tests/system/Config/ConfigTest.php:14

  1. CodeIgniter\Config\ConfigTest::testCreateSharedInstance
    Failed asserting that false is true.

/pub7/htdocs/CodeIgniter4/tests/system/Config/ConfigTest.php:31

Looking at the travis-ci results, it shows an error and four failures, but then echos "Hello World" at the end of the row after 1403/2089 and then exits with "success" !?

@jim-parry
Copy link
Contributor

ps - the error and two failures outside of ConfigTest show in other PRs, and were not caused by this PR.

@puschie286
Copy link
Contributor Author

Hmm, the tests passed locally - it’s pretty weird, not sure what’s the different between our installation that can cause this.

@puschie286 puschie286 restored the ConfigHelper branch June 28, 2018 18:04
@jim-parry
Copy link
Contributor

Are you perchance running on WIndows locally?

@puschie286
Copy link
Contributor Author

puschie286 commented Jun 28, 2018 via email

@jim-parry
Copy link
Contributor

jim-parry commented Jun 28, 2018

Oops. Config::get('email') will return a class on your system because WIndows is case-insensitive. Linux won't find it, hence the test fails.
Lines 10 & 28 in your test will not return a class on Linux.

@jim-parry
Copy link
Contributor

Better practice is to test on Linux, eg inside Windows bash shell or inside a VM (eg VirtualBox), to avoid things that Windows will let slip through. I use Linux natively, and I think Lonnie uses VirtualBox + Vagrant on MacOSX.

@puschie286
Copy link
Contributor Author

Oh, so i can’t handle lower case class names at all.
I will push the needed changes tomorrow.

@jim-parry
Copy link
Contributor

The alternative is to UCfirst the class name that results, which should work on all platforms.

@puschie286
Copy link
Contributor Author

puschie286 commented Jun 29, 2018

hmm, the internet says no -> php is not case sensitive for classes SO

i guess the filenames are the problem, because we are loading them directly.

update : #1088

@jim-parry
Copy link
Contributor

It is not a classname case sensitivity, it is a filename case sensitivity problem.
If you have a file AbCdEf.php, containing a PurpleDragon class definition, then you can load it as "abcdef.php" on Windows, but you need to use the correct filename case on Linux, or the source file will not be found.
Once loaded, you can then instantiate purpledragon or PURPLEdragon or whatever :-/
The example here is purposeful, to highlight the different issues.

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

3 participants