Skip to content

feat: custom exception handler - #6710

Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from
kenjis:feat-ExceptionHandler
Closed

feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from
kenjis:feat-ExceptionHandler

Conversation

@kenjis

@kenjiskenjis commented Oct 18, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Description
Supersedes #5675

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjiskenjis added enhancement PRs that improve existing functionalities 4.3 labels Oct 18, 2022
MGatner
MGatner reviewed Oct 18, 2022
View reviewed changes

@MGatnerMGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.

Comment threadsystem/Debug/BaseExceptionHandler.php Outdated
@lonnieezell

lonnieezell commented Oct 18, 2022

Copy link
Copy Markdown
Member

I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.

That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?

@kenjis

kenjis commented Oct 18, 2022

Copy link
Copy Markdown
MemberAuthor

No problem. The original PR was created many months ago.

I have tried to keep this PR free of BC breaks.

@kenjis
kenjisforce-pushed the feat-ExceptionHandler branch from b22c181 to 2017313CompareOctober 23, 2022 07:32
@kenjis

kenjis commented Oct 23, 2022

Copy link
Copy Markdown
MemberAuthor

Rebased.

@kenjis
kenjisforce-pushed the feat-ExceptionHandler branch 2 times, most recently from af3f99e to 9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner requested changes Oct 25, 2022
View reviewed changes

@MGatnerMGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:

  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.

Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
*
* @return bool|string
*/
protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)

@MGatnerMGatnerOct 25, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?

@kenjiskenjisOct 27, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

What do you mean?

Comment threadsystem/Debug/Exceptions.php Outdated

// For upgraded users.
if (! method_exists($this->config, 'handler')) {
$this->defaultExceptionHandler($exception, $statusCode, $exitCode);

@MGatnerMGatnerOct 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?

@kenjiskenjisOct 26, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Good point.
It should use ExceptionHandler.
When using ExceptionHandler, if the behavior changes it is a breaking change.

@kenjiskenjisOct 27, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

Comment threadsystem/Debug/ExceptionHandler.php Outdated
Comment threadsystem/Debug/ExceptionHandler.php
/**
* Determines the correct way to display the error.
*
* @return void

@MGatnerMGatnerOct 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?

@kenjis
kenjisforce-pushed the feat-ExceptionHandler branch from 9db24aa to b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed the feat-ExceptionHandler branch from b93407c to 9d5d44fCompareOctober 27, 2022 08:21
@kenjis

kenjis commented Oct 27, 2022

Copy link
Copy Markdown
MemberAuthor

No. If a dev overrides the following methods, this PR breaks the extended Exceptions:

  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()

@kenjiskenjis added the breaking change Pull requests that may break existing functionalities label Oct 27, 2022
@kenjis

kenjis commented Oct 27, 2022

Copy link
Copy Markdown
MemberAuthor

Reverting 9d5d44f and a dev does not update the Config class,
then there is no BC break even if the dev extends Exceptions.

@MGatner

MGatner commented Oct 27, 2022

Copy link
Copy Markdown
Member

I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?

@MGatner

MGatner commented Oct 27, 2022

Copy link
Copy Markdown
Member

No. If a dev overrides the following methods, this PR breaks the extended Exceptions

If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.

@kenjis

kenjis commented Oct 27, 2022

Copy link
Copy Markdown
MemberAuthor

I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?

The new classes do not need to be backwards-compatible with Exceptions.

A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,
this PR breaks the custom Exceptions.

If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.

It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.
Alternatively, it can also be determined by setting it to opt-in in the Config.

lonnieezell
lonnieezell reviewed Oct 27, 2022
View reviewed changes

@lonnieezelllonnieezell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.

Comment threadsystem/Debug/ExceptionHandler.php
$viewFile = null;
if (is_file($path . $view)) {
$viewFile = $path . $view;
} elseif (is_file($altPath . $altView)) {

@lonnieezelllonnieezellOct 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?

@kenjiskenjis added 4.4 and removed 4.3 labels Oct 28, 2022
@MGatner

MGatner commented Oct 28, 2022

Copy link
Copy Markdown
Member

The new classes do not need to be backwards-compatible with Exceptions.

Since this is the case please add explicit types to all the new class methods.

I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.

@MGatnerMGatner mentioned this pull request Oct 30, 2022
Merged
5 tasks
@kenjis
kenjis deleted the branch codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis closed this Jan 10, 2023
@kenjiskenjis mentioned this pull request Jan 11, 2023
Merged
5 tasks
@kenjis
kenjis deleted the feat-ExceptionHandler branch February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees

No one assigned

Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
feat: custom exception handler by kenjis · Pull Request #6710 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
feat: custom exception handler - #6710
Closed
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Closed
feat: custom exception handler#6710
kenjis wants to merge 17 commits into
codeigniter4:4.3from 
kenjis:feat-ExceptionHandler
Conversation
@kenjis
@kenjiskenjis
commented
Oct 18, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Description

Supersedes #5675
Checklist:
  •  Securely signed commits
  •  Component(s) with PHPDoc blocks, only if necessary or adds value
  •  Unit testing, with >80% coverage
  •  User guide updated
  •  Conforms to style guide
@kenjiskenjis
added

enhancement
PRs that improve existing functionalities
4.3

labels
Oct 18, 2022
MGatner
MGatner
reviewed
Oct 18, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.
Comment threadsystem/Debug/BaseExceptionHandler.php
Outdated

@lonnieezell
lonnieezell
commented
Oct 18, 2022

Copy link


Copy Markdown

Member
I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.
That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?
@kenjis
kenjis
commented
Oct 18, 2022

Copy link


Copy Markdown

MemberAuthor
No problem. The original PR was created many months ago.
I have tried to keep this PR free of BC breaks.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b22c181 to
2017313CompareOctober 23, 2022 07:32
@kenjis
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
Rebased.
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
2 times, most recently
from
af3f99e to
9db24aaCompareOctober 24, 2022 23:37
MGatner
MGatner
requested changes
Oct 25, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:
  1. It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
  2. I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.
Comment threadsystem/Debug/BaseExceptionHandler.php
Comment threadsystem/Debug/BaseExceptionHandler.php
 *
 * @return bool|string
 */
 protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)
@MGatnerMGatnerOct 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Since the existing handler doesn't actually extend this base, can we go ahead and type this?
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

What do you mean?
Comment threadsystem/Debug/Exceptions.php
Outdated


 // For upgraded users.
 if (! method_exists($this->config, 'handler')) {
 $this->defaultExceptionHandler($exception, $statusCode, $exitCode);
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?
@kenjiskenjisOct 26, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good point.

It should use ExceptionHandler.

When using ExceptionHandler, if the behavior changes it is a breaking change.
@kenjiskenjisOct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadsystem/Debug/ExceptionHandler.php
Outdated

Comment threadsystem/Debug/ExceptionHandler.php
 /**
 * Determines the correct way to display the error.
 *
 * @return void
@MGatnerMGatnerOct 25, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It feels like a big loss not having this in the abstraction. Any reason not to do that?
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
9db24aa to
b93407cCompareOctober 26, 2022 09:45
@kenjis
kenjisforce-pushed
the
feat-ExceptionHandler
branch
from
b93407c to
9d5d44fCompareOctober 27, 2022 08:21
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
No. If a dev overrides the following methods, this PR breaks the extended Exceptions:
  • Exceptions::render()
  • Exceptions::determineView()
  • Exceptions::collectVars()
  • Exceptions::maskSensitiveData()
@kenjiskenjis
added
the 
breaking change
Pull requests that may break existing functionalities
label
Oct 27, 2022
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
Reverting 9d5d44f and a dev does not update the Config class,

then there is no BC break even if the dev extends Exceptions.
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
No. If a dev overrides the following methods, this PR breaks the extended Exceptions
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
@kenjis
kenjis
commented
Oct 27, 2022

Copy link


Copy Markdown

MemberAuthor
I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?
The new classes do not need to be backwards-compatible with Exceptions.
A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,

this PR breaks the custom Exceptions.
If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.
It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.

Alternatively, it can also be determined by setting it to opt-in in the Config.
lonnieezell
lonnieezell
reviewed
Oct 27, 2022
View reviewed changes
@lonnieezelllonnieezell
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks pretty good to me. I just found one edge case that we don't handle.
Comment threadsystem/Debug/ExceptionHandler.php
 $viewFile = null;
 if (is_file($path . $view)) {
 $viewFile = $path . $view;
 } elseif (is_file($altPath . $altView)) {
@lonnieezelllonnieezellOct 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Seems like we need to handle if neither file exists?
@kenjiskenjis
added

4.4

and removed

4.3

labels
Oct 28, 2022
@MGatner
MGatner
commented
Oct 28, 2022

Copy link


Copy Markdown

Member
The new classes do not need to be backwards-compatible with Exceptions.
Since this is the case please add explicit types to all the new class methods.
I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.
@MGatnerMGatner
mentioned this pull request
Oct 30, 2022
 Merged

5 tasks
@kenjis
kenjis
deleted the branch
codeigniter4:4.3January 10, 2023 06:36
@kenjiskenjis
closed this
Jan 10, 2023
@kenjiskenjis
mentioned this pull request
Jan 11, 2023
 Merged

5 tasks
@kenjis
kenjis
deleted the

feat-ExceptionHandler

branch
February 8, 2023 01:37
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lonnieezelllonnieezelllonnieezell left review comments
@MGatnerMGatnerMGatner requested changes
@paulbalandanpaulbalandanAwaiting requested review from paulbalandan

Assignees


No one assigned


Labels

breaking changePull requests that may break existing functionalitiesenhancementPRs that improve existing functionalities

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@kenjis@lonnieezell@MGatner