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

[TASK] Avoid Hungarian notation in a class #704

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Conversation

oliverklee
Copy link
Contributor

This change does not include any breaking changes.

Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

According to the deprecation policy discussion - #453 (comment) - we should provide magic methods to access the new property names via the old names, marking the old names deprecated.

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 3, 2024

we should provide magic methods to access the new property names via the old names, marking the old names deprecated.

The PublicPropertyDeprecationTrait class from TYPO3 along with associated TestCase look ideal for the job, assuming the licence is compatible.

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 3, 2024

This change does not include any breaking changes.

The renamed properties are protected, so this would break any subclass accessing them directly.

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 3, 2024

The renamed properties are protected, so this would break any subclass accessing them directly.

I think we should also make them private as part of this change. (But do still need to deprecate them first.)

The PublicPropertyDeprecationTrait class from TYPO3 along with associated TestCase look ideal for the job

I think I've spotted a mistake in the class: __isset() should return isset($this->$propertyName) in all branches. (The other magic methods do the equivalent, and look correct.)

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 3, 2024

The PublicPropertyDeprecationTrait class from TYPO3 along with associated TestCase look ideal for the job

Looking again, this only provides for deprecation without renaming (i.e. changing to access to private). To support renaming and deprecation at the same time, there'd also need to be a map from old property names to new ones.

@oliverklee
Copy link
Contributor Author

Marking as draft for now - I think I'd like to change our policy for public APIs.

@oliverklee oliverklee marked this pull request as draft September 4, 2024 10:52
This change does not include any breaking changes.
@oliverklee oliverklee marked this pull request as ready for review September 20, 2024 12:17
@oliverklee
Copy link
Contributor Author

With our new API and deprecation policy in place, I think this is unblocked now.

@coveralls
Copy link

Coverage Status

coverage: 38.652%. remained the same
when pulling 80ceae6 on task/no-hungarian-6
into 76076de on main.

Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

With our new API and deprecation policy in place, I think this is unblocked now.

Yes, the API policy says that classes should not be extended, so protected methods are effectively internal.

@JakeQZ JakeQZ merged commit 3ba62f1 into main Sep 20, 2024
21 checks passed
@JakeQZ JakeQZ deleted the task/no-hungarian-6 branch September 20, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants