Skip to content

Error fixes in entity classes.#3

Merged
demiankatz merged 2 commits into
demiankatz:doctrinefrom
skellamp:doctrine
Feb 7, 2023
Merged

Error fixes in entity classes.#3
demiankatz merged 2 commits into
demiankatz:doctrinefrom
skellamp:doctrine

Conversation

@skellamp
Copy link
Copy Markdown

@skellamp skellamp commented Feb 3, 2023

TODO

Copy link
Copy Markdown
Owner

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @skellamp, this is an excellent start, and I appreciate the significant amount of work that you obviously put into this.

I've left a few comments -- there are some very small typos and minor suggestions about rephrasing descriptions. There are also a couple of areas where I had questions about Doctrine best practices. I also noticed some inconsistencies that, while probably not wrong, we might want to standardize to make sure we're treating the same situations in the same ways across all of the entities.

I hope this is helpful! I'm happy to discuss further as needed.

Comment thread module/VuFind/src/VuFind/Db/Entity/AccessToken.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Entity/AccessToken.php
Comment thread module/VuFind/src/VuFind/Db/Entity/AuthHash.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Entity/ChangeTracker.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Entity/Comments.php
Comment thread module/VuFind/src/VuFind/Db/Entity/User.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Entity/UserCard.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Entity/UserList.php Outdated

/**
* Flag to indicate whether or not the list is public.
*
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Might want to add TODO: update to boolean after merge of #2689 so we don't lose track of finishing that cleanup after the PR is completed.

Comment thread module/VuFind/src/VuFind/Db/Entity/UserResource.php Outdated
@demiankatz
Copy link
Copy Markdown
Owner

Also, please don't worry about the failing dependency review -- I think this has to do with permissions on the demiankatz repository vs. permissions on the vufind-org repository. The important CI checks are passing now, which is a great step forward. I don't expect we'll see any problems once we merge this into the main Doctrine PR.

@demiankatz demiankatz marked this pull request as ready for review February 7, 2023 19:30
Copy link
Copy Markdown
Owner

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

I have moved outstanding work from here into TODO checkboxes on vufind-org#2689, and the build is passing here, so I am merging now. Thanks again for your help cleaning all this up!

@demiankatz demiankatz merged commit 6a80481 into demiankatz:doctrine Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants