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

Codeigniter 4 fork #18

Closed
savioret opened this issue Nov 8, 2020 · 10 comments
Closed

Codeigniter 4 fork #18

savioret opened this issue Nov 8, 2020 · 10 comments

Comments

@savioret
Copy link
Contributor

savioret commented Nov 8, 2020

Hello,

I have created a fork of the project to adapt it to Codeigniter 4:
https://github.com/savioret/codeigniter-log-viewer

The current code is working in CI4 and I've updated the README.md with new instructions for CI4
But I'm not sure how to proceed with this fork, will you merge this (as a release v2.0.0 tag for example) or should I go on with the fork and create my own composer package pointing to my forked repo ?

Thanks.

@SeunMatt
Copy link
Owner

SeunMatt commented Nov 8, 2020

Hello @savioret

How are you today? Thanks for the update. Migrating it to version 4 of CI has been on my mind for a while but I've been busy.

So I appreciate your effort and work. Well done sir.

On the matter of how you should proceed. The ideal thing is for you to create a PR here so I can merge and release a new version, say 2.0.0, as you've rightly suggested. This is what I will like us to do.

However, the decision is 100% up to you. If you create a PR, I will dedicate the time to review and merge (that may take 2 weeks or less). On the other hand, releasing a version of your own fork is also cool.

Finally, I will like to clarify that either you submit a PR or release your own version, we will still update this one to work with CI4.

Let me know what your final decision is.

Cheers

@savioret
Copy link
Contributor Author

Hello Seun,
Sorry for the delay, I've been quite busy these 2 weeks.
Of course, I'd be pleased if you could merge my fork and continue from here.

The first thing you must know is that the fork can be considered as a pre-alpha version.
There are still things to test in depth (API calls, and try every different logging level, for instance).

Things to consider:

  • I've simplified the regex, now there's only one pattern
  • The log folder path defaults to 'writable/logs' (The same as defaults Logs\Handlers\FileHandler in the constructor). Instead of using a custom config value here, it will read App\Config\Logger::$path (which is already customizable) and will use that one if set.
  • Now the user does not need to copy the logs.php to views/cilogviewer` folder as we can make use of namespaced views
  • Therefore the App\Config\CILogViewer::$viewPath can be a path under views or a namespaced view (before it was set with CI_LOG_VIEW_FILE_PATH)
  • I've added the CRITICAL level but I was not sure what icon to use.
  • There may be more log levels to consider. The complete list of log levels is (from CI4's Logger class):
/*
    * The log levels that this handler will handle.
    */
'handles'         => [
    'critical',
    'alert',
    'emergency',
    'debug',
    'error',
    'info',
    'notice',
    'warning',
    ],

If there anything that was changed that you don't feel comfortable with, or you don't know why I changed it or simplified it, we can just discuss it.
So, if I'm going to proceed with the PR, what branch should I pull to ?

Thanks.

@marcianomxz
Copy link

As a newbie, I hope the version can be available for the Codeigniter 4, sure it will help us a lot.

@gopibabus
Copy link

Hi @savioret and @SeunMatt . I didn't see any activity on this request since November 2020. Can I expect this package is updated with codeigniter4 changes?

If not, I will create a fork from @savioret changes and publish as new package for codeigniter4. Please let me know your thoughts. Thanks.

@SeunMatt
Copy link
Owner

Hello @gopibabus, @savioret didn't create a PR eventually. I'll look into upgrading this to work with CI4 myself. Please check back by the 30th of this month (September 2022) to see if there's any progress from my end.

Best Regards

@gopibabus
Copy link

Sure. Thanks @SeunMatt .

@savioret
Copy link
Contributor Author

savioret commented Sep 19, 2022

Hi, I was waiting @SeunMatt to give directions on which branch to merge to.
Anyway I've created a pull request to development branch as I've seen it's been recently sync with main.
Please review my previous comments explaining why I changed some things in this fork, that I thought were more appropiate to work with version 4.
Thanks.

@gopibabus
Copy link

Hi @SeunMatt , Can you review the @savioret PR?

@SeunMatt
Copy link
Owner

SeunMatt commented Oct 8, 2022

Hello, @gopibabus and @savioret PR has been reviewed, waiting for feedback from @savioret.
Regards

@SeunMatt
Copy link
Owner

SeunMatt commented Oct 9, 2022

Hello @gopibabus and @savioret v2 has been released which supports CI4. Thanks for the good work everyone, special kudos to @savioret for the PR. Please test and provide feedback.
I'll be closing this issue now.

Cheers 🥂

@SeunMatt SeunMatt closed this as completed Oct 9, 2022
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

No branches or pull requests

4 participants