search

nextcloud / cms_pico

🗃 Integrate Pico CMS and let your users manage their own websites
https://apps.nextcloud.com/apps/cms_pico
GNU Affero General Public License v3.0
137 stars 43 forks source link

fix: replace logger #251

Closed vitormattos closed 3 weeks ago

vitormattos commented 1 month ago

The previous logger approach was removed at Nextcloud 30 and was needed to be replaced by Psr\Log\LoggerInterface

This PR fix:

Error: Call to undefined method OC\Server::getLogger() in /var/www/html/apps-writable/cms_pico/lib/Migration/Version010000From000908.php:65
PhrozenByte commented 1 month ago

Thank you for your contribution! :heart:

First of all, please note that cms_pico is abandoned dev-wise, thus there isn't going to be a new release in the foreseeable future. There's currently no stable version of cms_pico supporting any of Nextcloud's still officially supported branches; people still using cms_pico do so at their own risk and use custom app builds (usually) based on the cms_pico-1.0 branch.

You can ignore the failing CI, just wanted to see whether it's more broken than last time I checked - it indeed is.

If you're willing to invest more time nevertheless, can you please rebase on the cms_pico-1.0 branch? The master branch is the dev version of cms_pico v2.0. Just switching the PR's base unfortunately doesn't work properly. Did you check all usages of $logger to be compatible with the new LoggerInterface? Unfortunately I don't have the time to do a proper review of your PR, but the changes look good; so, if you affirm that you did check all usages, I'll merge your PR nevertheless. Thanks again!

vitormattos commented 1 month ago

Last changes:

github-actions[bot] commented 4 weeks ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:

PhrozenByte commented 4 weeks ago

Did you check all usages of $logger to be compatible with the new LoggerInterface?

Did you check these? If yes, I'll merge it right away. Thanks again for your contribution! :+1:

vitormattos commented 3 weeks ago

Yes, I didn't made smoke test looking all places of code that use the old log interface, but the piece of code that I tested worked fine and because this I made the replace at all places.

I took care to check the name of methods, because this the place that the code used LogException method, I replaced by a most similar method from LoggerInterface, the other places, the method called have the same name.

PhrozenByte commented 3 weeks ago

Alright, thanks again for your contribution! :+1: