Additional Cleanup for Building in PHP 7.0 #183
Conversation
Update from latest changes
Modular Acls: ETL Support (ubccr#97)
|
lemme put this on hold, run_tests.sh seems to work fine. |
|
Looks like a style issue. Run PHPCS on any files that you modified (E.g., |
configuration/linker.php
Outdated
| @@ -15,6 +15,7 @@ | |||
| // Register a custom autoloader for XDMoD components. | |||
| $include_path = ini_get('include_path'); | |||
| $include_path .= ":" . $baseDir . '/classes'; | |||
| $include_path .= ":" . $baseDir . '/classes/CCR'; | |||
There was a problem hiding this comment.
I don't think that we want to make this change to the linker. This effectively changes the interpretation of the namespaces, which I don't think is a good idea.
There was a problem hiding this comment.
If you have files that are incorrectly referencing the wrong namespace then please fix those files.
configuration/linker.php
Outdated
| $logConf = array('mode' => 0644); | ||
| $logger = Log::factory('file', $logfile, 'exception', $logConf); | ||
| $logConf = array('mode' => 0644); | ||
| $logger = Log::factory($logfile, $logConf); |
There was a problem hiding this comment.
Why did you change this function call? Originally the code used the PEAR::Log factory function to generate a file logger. Now you have changed it to use a CCR::Log factory function that does something completely different. Please revert this change.
classes/CCR/Log.php
Outdated
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| namespace CCR; | |||
|
|
|||
| require_once 'System.php'; | |||
There was a problem hiding this comment.
What bug this this change intended to fix?
| @@ -18,3 +18,5 @@ build: | |||
| - composer install --no-progress | |||
| - cp ~/assets/secrets open_xdmod/modules/xdmod/integration_tests/.secrets | |||
| - ./open_xdmod/modules/xdmod/integration_tests/runtests.sh --log-junit `pwd`/shippable/testresults/results.xml | |||
| on_failure: | |||
| - cat /var/log/xdmod/* | |||
There was a problem hiding this comment.
there should be a newline at the end of this file.
|
As of PHP 7.1.x, looks like they replaced InvalidArgumentException with ArgumentCountError (REF: http://php.net/manual/en/class.argumentcounterror.php). This causes IdentityTest::testNoDefaultParameterToConstructor() to look like it's failing, even though it's just using a different exception than prior versions. Since fixing this would break the test's compatibility for versions we presently support, I'd say accept it for now. Thoughts? |
|
From the docs: |
|
Sorry, not redundant in the sense that there's a test that does something similar, it just seems to be testing a feature of the language itself rather than any code specific to xdmod. It can be assumed that when you initialize a constructor without a parameter (where the constructor specifies for one), that it should fail. So I see it as redundant in that it doesn't seem to test anything specific. |
|
I don't think removing the entire set of tests is a good way to deal with the exception changes in PHP7. There are other valid tests in that file for the Identity class that should not be removed. Even |
Description
Added explicit requires, removed unused 'external_libraries' reference that was breaking the build. Should be pretty minor changes all around, mostly involved cleanup. Only noteworthy change I can think of was removing some concatenation with variables outside of the context in which they'd be known (Line 93, configuration/linker.php)
Motivation and Context
Cleanup to get it to build properly with PHP 7.0
Tests performed
Rebuilt XDMOD + ran tests in the open_xdmod module
Types of changes
Checklist: