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

Remove Log.php require_once statements #13842

Merged
merged 2 commits into from
Mar 17, 2019

Conversation

seamuslee001
Copy link
Contributor

Overview

Removes now unnecessary require_once statements for Log.php files

ping @totten @eileenmcnaughton

@civibot
Copy link

civibot bot commented Mar 16, 2019

(Standard links)

@civibot civibot bot added the master label Mar 16, 2019
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

ping @totten seems to have passed now we have merged the composer and the removal of the original log files

@seamuslee001
Copy link
Contributor Author

@totten given the issue we were seeing in the test earlier i have added in the Log files to the cleanup files list so that hopefully there are no clash issues on Joomla

@eileenmcnaughton
Copy link
Contributor

Agree this makes sense & we have definitely merged enough parts of this switch we are committed :-)

@eileenmcnaughton eileenmcnaughton merged commit 2d5479d into civicrm:master Mar 17, 2019
@eileenmcnaughton eileenmcnaughton deleted the upgrade_log_package branch March 17, 2019 20:30
@colemanw
Copy link
Member

Ok but how is the new vendor/pear/log/Log.php file getting included? I don't see any include statement that refers to it. Don't we need to alter these require_oncees to point to the new vendor/pear/log/Log.php instead of just removing them?
@eileenmcnaughton @seamuslee001 @totten see bug report in #13835

@eileenmcnaughton
Copy link
Contributor

No idea but the revert works....

@seamuslee001
Copy link
Contributor Author

@colemanw no because composer is including the library’s folder in its psr-0 autoloader

@seamuslee001
Copy link
Contributor Author

Also note since this was merged I have run another ext matrix and api4 tests pass

@colemanw
Copy link
Member

@seamuslee001 is there something I need to do in my local sandbox other than cv flush to make the new include work? I'm still having trouble locally.

@seamuslee001
Copy link
Contributor Author

@colemanw make sure you have run composer install based on the latest master that should hopefully take care of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants