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

[Core] Update NDB_Page constructor to include LorisInstance object #7414

Merged
merged 10 commits into from
Apr 28, 2021

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Mar 29, 2021

This updates the NDB_Page constructor to include a LorisInstance object. The LorisInstance object represents the properties of the LorisInstance itself (such as the database connection) and can replace many factory/singleton calls, but is seldom used in LORIS because it's difficult to get a copy of the object. (It needs to be indirectly accessed from a PSR7 request attribute). After moving it to the constructor, it can now be accessed directly from any NDB_Page class or subclass in a simple class property.

I was originally planning to clean up the NDB_Page constructor more by removing unnecessary parameters (why does every page get a CommentID parameter? Why is there both CommentID and identifier? Why does the class need to be told its own name? etc) but simply adding one argument and propagating it to all the places that needed to be updated was already a fairly large PR. This also makes some other cleanup possible, such as moving other statics like \Module::factory to $loris->getModule() but, once again, that would increase the size of this PR more than I'd like.

@driusan
Copy link
Collaborator Author

driusan commented Mar 29, 2021

Blocked by #7412

@driusan driusan added State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed Cleanup PR or issue introducing/requiring at least one clean-up operation labels Mar 29, 2021
@driusan driusan force-pushed the OnlySlightBetterPageConstructor branch 2 times, most recently from 5c8c063 to 0736a96 Compare March 29, 2021 19:09
@driusan driusan force-pushed the OnlySlightBetterPageConstructor branch from 0736a96 to 51db7b4 Compare April 20, 2021 18:41
@driusan driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Apr 20, 2021
@driusan driusan force-pushed the OnlySlightBetterPageConstructor branch from 9a54416 to c3e86e1 Compare April 22, 2021 18:59
Copy link
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

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

LGTM, and passing test

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Apr 28, 2021
@driusan driusan merged commit 8568d0d into aces:main Apr 28, 2021
laemtl pushed a commit to laemtl/Loris that referenced this pull request Jun 7, 2021
…ces#7414)

This updates the NDB_Page constructor to include a LorisInstance object. The LorisInstance object represents the properties of the LorisInstance itself (such as the database connection) and can replace many factory/singleton calls, but is seldom used in LORIS because it's difficult to get a copy of the object. (It needs to be indirectly accessed from a PSR7 request attribute). After moving it to the constructor, it can now be accessed directly from any NDB_Page class or subclass in a simple class property.
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
…ces#7414)

This updates the NDB_Page constructor to include a LorisInstance object. The LorisInstance object represents the properties of the LorisInstance itself (such as the database connection) and can replace many factory/singleton calls, but is seldom used in LORIS because it's difficult to get a copy of the object. (It needs to be indirectly accessed from a PSR7 request attribute). After moving it to the constructor, it can now be accessed directly from any NDB_Page class or subclass in a simple class property.
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants