[TASK] Mark parsing-internal classes and methods as @internal
#674
Conversation
oliverklee
added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone
There was a problem hiding this comment.
Anchor and ParserState both look internal to me, so marking those as internal seems fine.
I'm not sure if the exception class hierarchy should be internal.
The Parser::get/setCharset methods are part of the current interface (and not actually used internally). If we want to remove them (I'm not sure), we should mark them as deprecated.
@sabberworm, what are your thoughts on these?
I thought the plan to mark methods as deprecated required the logger interface to be in place first, so that users can be informed if they are calling deprecated methods. So if we do want to deprecate the charset get/set methods, that part of this PR would have to be put on hold.
src/Parser.php
Outdated
| * Sets the charset to be used if the CSS does not contain an `@charset` declaration. | ||
| * | ||
| * @param string $sCharset | ||
| * | ||
| * @internal | ||
| */ | ||
| public function setCharset($sCharset): void |
There was a problem hiding this comment.
The description suggests that this is not an internal method. However, the example in the readme shows setting the charset via Settings::withDefaultCharset(). But searching the code base, I find this is not actually called internally (internally we set the charset directly on the ParserState object).
So if we don't want to expose this method moving forwards, we should mark it as deprecated. In which case, we want the logger inferface implementation before we can do so, don't we?
There was a problem hiding this comment.
They are now deprecated (#688) instead.
src/Parsing/OutputException.php
Outdated
| * Thrown if the CSS parser attempts to print something invalid. | ||
| * | ||
| * @internal | ||
| */ | ||
| class OutputException extends SourceException |
There was a problem hiding this comment.
This is thrown on attempting to render a declaration block whose selector(s) have all been removed. There's a test specifically for that situation and behaviour (with or without the 'throw exceptions' setting), so this doesn't look internal to me.
The same applies to the entire exception hierarchy. Though I don't see a reason not to make them internal moving forwards..
No, I don't think that not having the logger should be a blocker for deprecating things. |
I have now created #688 to deprecate them. |
oliverklee
force-pushed
the
task/internal-parser
branch
from
58b9182
to
748a15c
Compare
Code that uses this library is not expected to call internal parsing functionality. Communicate this with the corresponding `@internal` annotation. This allows us to boldly refactor the parser code. Part of #668
oliverklee
force-pushed
the
task/internal-parser
branch
from
748a15c
to
c826324
Compare
|
Updated and repushed. |
|
This is still marked as 'Draft'. |
Oops, thanks - not anymore! 😉 |
Code that uses this library is not expected to call internal parsing functionality. Communicate this with the corresponding
@internalannotation.This allows us to boldly refactor the parser code.
Part of #668