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.

Sign up for GitHub

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

Jump to bottom

6.x: rename _accessible to patchable #18039

Open
wants to merge 1 commit into
base: 6.x
Choose a base branch
from
Open

6.x: rename _accessible to patchable #18039

wants to merge 1 commit into from

Conversation

LordSimal
Copy link
Contributor

Resolves #17375

The following Rector configs need to be created to help users upgrade to this new name:

  • Rename EntityInterface methods
    • setAccess => setPatchable
    • getAccessible => getPatchable
    • isAccessible => isPatchable
  • Rename EntityTrait property _accessible => patchable
  • Rename Marshaller/ORM method option accessibleFields => patchableFields

@LordSimal LordSimal added the ORM label Nov 24, 2024
@LordSimal LordSimal added this to the 6.0 milestone Nov 24, 2024
@LordSimal LordSimal force-pushed the 6.x-patchable-prop branch 2 times, most recently from deb5419 to 07fc240 Compare November 24, 2024 20:17
LordSimal
LordSimal commented Nov 24, 2024
View reviewed changes
src/ORM/phpstan.neon.dist Outdated Show resolved Hide resolved
ADmad
ADmad reviewed Nov 25, 2024
View reviewed changes
src/ORM/Marshaller.php
* the accessible fields list in the entity will be used. Defaults to null.
* - accessibleFields: A list of fields to allow or deny in entity accessible fields. Defaults to null
* the patchable fields list in the entity will be used. Defaults to null.
* - patchableFields: A list of fields to allow or deny in entity patchable fields. Defaults to null
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we could shorten this to just patchable. The Fields suffix seems unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

patchable does sound a bit like a boolean flag, though.
But I can understand that the length might be a bit cumbersome.

@ADmad ADmad requested a review from markstory December 3, 2024 11:28
markstory
markstory reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

I like the new method names and property names. My only concern is the amount of toil this will add to the 6.x upgrade. We'll need to make sure folks are aware of the upgrade tooling more than in the past 😄

@LordSimal
Copy link
Contributor Author

We'll need to make sure folks are aware of the upgrade tooling more than in the past 😄

Then I will make a Youtube Video showcasing the Upgrade tool once again (since no one seems to know I showcased it in CakeFest 2023)

Written docs are already present and maybe need a few touch ups in the end.

@ADmad
Copy link
Member

ADmad commented Dec 4, 2024 •
edited
Loading

The change in methods name would be easy to spot if one misses the rename as it would just generate an error. The array key change would be silently ignored if missed. So for those we should keep a check and show a warning.

@LordSimal LordSimal mentioned this pull request Dec 26, 2024
Merged
@dereuromark
Copy link
Member

Looks good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADmad ADmad ADmad left review comments

@dereuromark dereuromark dereuromark left review comments

@markstory markstory markstory left review comments

Assignees
No one assigned
Labels
ORM
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@LordSimal @ADmad @dereuromark @markstory