Skip to content

Rename Comment.comment #450

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

Open
oliverklee opened this issue Feb 6, 2024 · 5 comments
Open

Rename Comment.comment #450

oliverklee opened this issue Feb 6, 2024 · 5 comments

Comments

@oliverklee
Copy link
Collaborator

Maybe Comment.contents or Comment.text?

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 6, 2024

Maybe Comment.contents or Comment.text?

DOMDocument uses textContent, which suggests using sContent here to be consistent with both that and the Hungarian notation used in this project.

@oliverklee
Copy link
Collaborator Author

I'd like to get rid of the Hungarian notation sooner or later (now that we can use native type declarations and PHPDoc annotations). However, how we name the property isn't that important as long as we encapsulate it. So should we name the getter/setter getTextContent and setTextContent?

@sabberworm
Copy link
Collaborator

sabberworm commented Feb 7, 2024

textContent in DOM is a very generic property, applicable to all sorts of node types, not just comments. I would be fine with either text or contents but I also don’t really see a problem with keeping comment TBH.

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 7, 2024

My concern with renaming properties and methods, particularly if they are public (though also if they are protected), is that it would be a breaking change.

PHPUnit renamed some of the assertion methods around version 8 or 9. (IIRC the old names were deprecated in version 8, and removed in version 10.) This prevented us using the latest version when testing with the latest PHP, while we still needed older versions compatible with older PHP versions we still supported (without special-casing the version of PHPUnit and calling the appropriate method for the version, which we did not do). WordPress has occasionally done similar renames, though does generate PHP warnings when deprecated functions and methods are called.

In short: such renames generate extra work for users wishing to upgrade, and can also result in dependency and version-compatibility problems that are difficult to resolve.

We'd first have to mark the old method/property as deprecated, with its implementation chaining on to the newly named equivalent (for properties, the magic __get and __set can be used). Then in some later release (perhaps 3 years later), finally remove the deprecated one.

The question I'd ask: Does the overall cost (including that to users) perhaps outweigh the benefit?

@oliverklee
Copy link
Collaborator Author

oliverklee commented Feb 8, 2024

I have moved this to a discussion thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants