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

Configuration should not be internal #4376

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Conversation

BenMorel
Copy link
Contributor

@BenMorel BenMorel commented Oct 21, 2020

Q A
Type improvement
BC Break no
Fixed issues none

Summary

The Configuration class is currently marked as @internal, even though this markup was actually intended to be used as an internal note, not for marking the whole class for internal use.

I think the phpdoc documentation is unclear on the subject:

The @internal tag is used to denote that associated Structural Elements are elements internal to this application or library. It may also be used inside a long description to insert a piece of text that is only applicable for the developers of this software.

But as a matter of fact, Psalm considers the class internal and reports a violation:

ERROR: InternalMethod - src/Bootstrap.php:305:26 - The method Doctrine\DBAL\Configuration::setSQLLogger is internal to Doctrine but called from Acme\Bootstrap (see https://psalm.dev/175)
$config->setSQLLogger(new FileLogger(__DIR__ . '/../sql-log.txt'));

I think we should be on the safe side and only use @internal to mark a class or method for internal use, and not for internal notes that can easily be formulated in other ways.

greg0ire
greg0ire previously approved these changes Oct 22, 2020
@greg0ire greg0ire requested a review from morozov October 22, 2020 06:23
@greg0ire
Copy link
Member

I'll retarget on 2.12.x because we are not going to have another 2.11 release

@greg0ire greg0ire changed the base branch from 2.11.x to 2.12.x October 22, 2020 06:26
@greg0ire greg0ire dismissed their stale review October 22, 2020 06:26

The base branch was changed.

@morozov morozov added this to the 2.12.0 milestone Oct 22, 2020
@morozov morozov merged commit a79c8c3 into doctrine:2.12.x Oct 22, 2020
@morozov
Copy link
Member

morozov commented Oct 22, 2020

Thanks, @BenMorel.

rgrellmann added a commit to Rossmann-IT/dbal that referenced this pull request Mar 7, 2021
Release [2.12.0](https://github.com/doctrine/dbal/milestone/82)

2.12.0
======

- Total issues resolved: **1**
- Total pull requests resolved: **7**
- Total contributors: **5**

Documentation,Static Analysis
-----------------------------

 - [4376: Configuration should not be internal](doctrine#4376) thanks to @BenMorel

CI
--

 - [4374: Reduce number of build jobs](doctrine#4374) thanks to @greg0ire
 - [4365: Fail on extension / tool installation failure](doctrine#4365) thanks to @greg0ire

Bug,Static Analysis
-------------------

 - [4373: Psalm fails on release commits](doctrine#4373) thanks to @morozov

Documentation,Error Handling
----------------------------

 - [4362: Adds exception thrown by execute() method](doctrine#4362) thanks to @toby-griffiths

CI,PHP
------

 - [4361: Test all extensions with PHP8](doctrine#4361) thanks to @greg0ire

PHP
---

 - [4347: &doctrine#91;2.12&doctrine#93; PHP 8 compatibility](doctrine#4347) thanks to @derrabus

# gpg: Signature made Thu Oct 22 20:29:24 2020
# gpg:                using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132
# gpg: Can't check signature: No public key
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants