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

Declare classes as final whenever possible #534

Merged
merged 4 commits into from
Sep 17, 2021

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Sep 17, 2021

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Code quality improvement

Context

Improve maintainability and make the interface of what are breaking changes smaller.

Fixes #514

Detailed Description

Tests: declare base TestCase as abstract

Tests: make all concrete test classes final

... and make all protected methods within them private.

Exceptions: make all concrete Exception classes final

Src: make select concrete classes final

... and make all protected methods within them private and when the class doesn't extend or implement, the properties too.

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

... and make all `protected` methods within them `private`.
... and make all `protected` methods within them `private` and when the class doesn't extend or implement, the properties too.
@schlessera schlessera merged commit 7e972c3 into develop Sep 17, 2021
@schlessera schlessera deleted the feature/514-declare-classes-as-final branch September 17, 2021 09:47
jrfnl added a commit that referenced this pull request Nov 5, 2021
In PR #534 (to address #514), a number of classes were made `final`. This effectively broke the BC-layer for the Composer autoloader as that BC-layer relied on the "old" class names extending the new ones.

Fixed now by:
* Using the new custom autoloader - which uses class aliasing, which is not affected by the change to `final` - for the BC-layer for Composer.
* Automatically loading the file via the composer `autoload` directive.

While using the `files` directive in Composer `autoload` is generally a last resort, as this file only registers an autoloader and Composer autoload is used for just that purpose, this should not be a problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check which classes can be changed to final
2 participants