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

Support iteration for DAO results #5122

Closed
asmecher opened this issue Oct 2, 2019 · 20 comments
Closed

Support iteration for DAO results #5122

asmecher opened this issue Oct 2, 2019 · 20 comments
Milestone

Comments

@asmecher
Copy link
Member

asmecher commented Oct 2, 2019

Currently the service classes often return arrays from getMany-type calls, which is convenient but requires all results to be loaded in-memory at once. This can be too resource-intensive, e.g. for long lists of submissions.

Instead, introduce a pattern for using an Iterator implementation. This will allow for foreach-based traversal with one-at-a-time instantiation.

asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 2, 2019
asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 3, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 3, 2019
@asmecher
Copy link
Member Author

asmecher commented Oct 3, 2019

PRs:

@NateWr, mind taking a look? Hopefully this substantiates what I was raving about in Slack.

@asmecher asmecher added this to the OJS/OMP 3.2 milestone Oct 3, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 3, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 3, 2019
@asmecher
Copy link
Member Author

asmecher commented Oct 3, 2019

Tweaked your suggestions -- just missing your blessing for the greater approach :)

asmecher added a commit to asmecher/pkp-lib that referenced this issue Oct 3, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 3, 2019
asmecher added a commit to asmecher/ojs that referenced this issue Oct 3, 2019
@NateWr
Copy link
Contributor

NateWr commented Oct 4, 2019

Ok, I've slept on it and here are my thoughts.

  1. Yes, let's go with an Iterator. Or a Generator if you think it will be more clear about the rewind limitation. I would say lets stick with what seems to be the most conventional approach in modern PHP.
  2. Let's change the name in the EntityReadInterface from getMany to getIterator or getGenerator. I think that will make it clearer that we're not getting a collection of objects.
  3. If we use a generator instead of an iterator, can we still use iterator_to_array()? If not, let's write a method like Generator::toArray() which we can use when needed.
  4. Once this is done, let's check each Hook to see where this might impact what's being passed down, and decide on a case-by-case basis how we want that Hook to be consumed: either a) convert the result set to an array, b) remove the result set completely so that they have to re-fetch the data, or c) move the position of the hook into the loop so that instead of passing a collection of objects the hook is fired separately for each object.

Does that sound alright? Thanks for talking me through all of this.

@asmecher
Copy link
Member Author

asmecher commented Oct 9, 2019

Yes, let's go with an Iterator. Or a Generator if you think it will be more clear about the rewind limitation. I would say lets stick with what seems to be the most conventional approach in modern PHP.

I think I'll stick with Iterator. The Generator interface extends Iterator anyway, so clearly the PHP dev team -- for what it's worth -- thinks non-rewindable Iterators are OK.

Let's change the name in the EntityReadInterface from getMany to getIterator or getGenerator. I think that will make it clearer that we're not getting a collection of objects.

I'm not so sure about this one; if we standardize to always returning iterators, and since iterators work with e.g. the foreach primitive, then I'm not sure it's worth disruptively removing getMany in favour of a new method name. It's true that we'd be changing the return value to a more restrictive type, but e.g. attempting to re-iterate [so to speak] will throw an Exception, so I think it's a lesser evil.

If we use a generator instead of an iterator, can we still use iterator_to_array()? If not, let's write a method like Generator::toArray() which we can use when needed.

Moot, since I'm OK with using an Iterator. But yes, Generators should still support iterator_to_array.

Once this is done, let's check each Hook...

Yes, can do.

@NateWr
Copy link
Contributor

NateWr commented Oct 10, 2019

Ok, I give in. Let's stick with getMany(). 😄

asmecher added a commit to asmecher/ojs that referenced this issue Oct 10, 2019
NateWr added a commit to NateWr/ojs that referenced this issue Oct 22, 2019
asmecher added a commit that referenced this issue Oct 22, 2019
#5122 Fix empty DAOResultIterator checks and adjust naming pattern
asmecher pushed a commit to pkp/omp that referenced this issue Oct 22, 2019
asmecher pushed a commit to pkp/ojs that referenced this issue Oct 22, 2019
@asmecher
Copy link
Member Author

All merged -- thanks, @NateWr! Personally I prefer e.g. $submissions over $results (more descriptive) but no big deal either way.

@NateWr
Copy link
Contributor

NateWr commented Oct 23, 2019

Personally I prefer e.g. $submissions over $results (more descriptive) but no big deal either way.

Yes, I'm torn on this as it seems easier to read with $submissions. But I also ran into exactly that confusion during the pair programming session because I made the assumption that I could call empty($submissions).

I'd be happy to also call it $submissionsIterator or something too, but I think, for myself at least, calling it $submissions will be a recurrent source of bugs from lack of attention.

@asmecher
Copy link
Member Author

Agreed, with a slight preference for $submissionsIterator (though it's long). I fear this stems from one of those legitimate criticisms of PHP.

However, if we're running a complex SELECT in order to just COUNT the results (or check if there were any), that's a wasteful pattern -- maybe there's some underlying work there.

@NateWr
Copy link
Contributor

NateWr commented Oct 24, 2019

However, if we're running a complex SELECT in order to just COUNT the results (or check if there were any), that's a wasteful pattern -- maybe there's some underlying work there.

Usually we're not just counting the results, but checking if there were 0 results before doing something else. (For example, when getting the list of issues, we check if there were any future issues before adding the "---Future Issues----" option to the select field.)

@NateWr
Copy link
Contributor

NateWr commented Oct 24, 2019

Ok, updated to $submissionsIterator.

PRs:
#5221
pkp/ojs#2516
pkp/omp#719

@NateWr NateWr reopened this Oct 24, 2019
asmecher added a commit that referenced this issue Oct 24, 2019
#5122 Change naming pattern for methods returning iterators
asmecher added a commit to pkp/ojs that referenced this issue Oct 24, 2019
pkp/pkp-lib#5122 Change naming pattern for methods returning iterators
asmecher added a commit to pkp/omp that referenced this issue Oct 24, 2019
 pkp/pkp-lib#5122 Update naming pattern when returning iterators
@asmecher
Copy link
Member Author

Yup, thinking about this further and seeing the change rolled out through the applications, this was definitely a worthwhile clarification. Thanks, @NateWr.

@NateWr NateWr closed this as completed Oct 25, 2019
NateWr added a commit to pkp/omp that referenced this issue Oct 25, 2019
yammut added a commit to yammut/ojs that referenced this issue Oct 29, 2019
* pkp/pkp-lib#5021 Restore subscription grid search options

* Complete es_ES locale for CrossRef plugin

Added missing translations for es_ES locale.

* pkp/pkp-lib#5023 Fix obsolete constant name

* Address Github security warnings

* pkp/pkp-lib#5015 Changed lang attribute for local keys

* pkp/pkp-lib#5015 Removed extra quotation mark

* Update copyright date

* fix typo in ru_RU locale

* Submodule update

* pkp/pkp-lib#5029 Bump PHP baseline to PHP7.2

* Bump citationStyleLanguage baseline to PHP7.2

* pkp/pkp-lib#5029 Submodule update ##asmecher/i5029-fix##

* pkp/pkp-lib#5029 Disable PHP7.1 Travis tests

* Complete API es_ES locales

Added all missing texts for es_ES in api.xml

* Submodule update

* Minor fixes (proposals)

Updated to september 4, 2019

* sl_SI-3_1_2_update

* Correct XML path for validation

* pkp/pkp-lib#2072 Working prototype of versioning based on new publication entity

This commit includes all of the initial work done to support versioning based on a split between submissions and publications. All submission data related to publication, such as title, abstract, citations, authors and galleys, has been moved to a new publication entity.

Submissions have a "one to many" relationship to Publications. Each Submission may have one or more Publications attached to it. Each Publication is treated as a new version. Published version data can not be modified.

- New publication entity split from submissions
- New API endpoints for publications
- Workflow UI changes to support versions (publications)
- Pre-publication validation checks
- New STATUS_SCHEDULED for publications scheduled for publication in a future issue
- Deprecated many methods on the Submission object
- Upgrade scripts written from 3.1.x.
- Tests updated to work, except for issue import.

Some code is commented out or has not been updated yet. Progress on remaining support for versioning will be tracked in Github.

See: https://github.com/pkp/pkp-lib/projects/15

* pkp/pkp-lib#2072 Fix whitespace and typo errors

* pkp/pkp-lib#2072 Fix conflict with citation style language in template variable name

* pkp/pkp-lib#2072 Fix tests

* pkp/pkp-lib#2072 Drop temporary table during upgrade

* Submodule update ##NateWr/i2072_versioning##

* pkp/pkp-lib#2072 Fix fatal error when previewing article not assigned to issue

* Submodule update

* pkp/pkp-lib#5057 Update MEDRA dev endpoint URL

* pkp/pkp-lib#5055 Fix author dashboard

* Submodule update

* Submodule update

* pkp/pkp-lib#5068 Cast sequences to integers

* Submodule update

* Submodule update

* Submodule update

* Update ru_RU locale after pkp#2457

* allow gateway plugins to add authorization policies

* Permit correct storage of section ID on initial insert

* Submodule update

* pkp/pkp-lib#5017 Include submission subtitle in Crossref XML

* Submodule update

* pkp/pkp-lib#4325 include affiliations for all authors

* Remove unused variable

* Recompile JS

* pkp/pkp-lib#4989 Add defaultReviewMode setting on upgrade

* Remove dead code

* Submodule update

* pkp/pkp-lib#5047 Don't allow galleys to be added or edited in published publications

* Submodule update ##NateWr/i5047_galley_edit##

* pkp/pkp-lib#5045 Restore keywords variables to article details template

* Update pt_BR REVISED_VERSION_NOTIFY email template

* pkp/pkp-lib#5089 fix variable test for section editor count

* pkp/pkp-lib#4870 Support versioning on article landing page

* Submodule update ##NateWr/i4870_reader_versioning##

* pkp/pkp-lib#5087 Don't show category selection if no categories exist

* pkp/pkp-lib#3386 Add new EditorialActionsHandler to minified scripts

* Submodule update ##NateWr/i3386_editorial_actions##

* Code syntax tweak

* pkp/pkp-lib#4906 Remove OAI dependency on published_submissions

* Revert 979337f

* pkp/pkp-lib#5103 Remove sexist language

* pkp/immersion#25 Move language-specific text into locale files

* pkp/immersion#25 Fix test language (on disabled test)

* pkp/immersion#25 Fix test language

* Correct typo

* pkp/pkp-lib#5044 Add date published field to journal entry form

- Change date_published column from datetime to date
- Remove unused publicationType and publicationDateType properties

* Submodule update ##NateWr/i5044_date_published##

* pkp/pkp-lib#5046 Add unpublish button to versions

* pkp/pkp-lib#4779 Move to using XLIFF files for translation (work in progress)

* pkp/pkp-lib#4779 Remove translator plugin

* pkp/pkp-lib#4779 Adapt to PO files instead of XLIFF

* pkp/pkp-lib#4779 Convert selected locales to PO

* pkp/pkp-lib#4779 Submodule update ##asmecher/po##

* pkp/pkp-lib#4779 Convert selected plugin locale files to PO

* pkp/pkp-lib#4779 Submodule update

* pkp/pkp-lib#4779 Convert pt_BR to PO format

* Submodule update

* Submodule update

* pkp/pkp-lib#5045 Indicate publishing schedule in publish confirmation message

* pkp/pkp-lib#5045 Use correct phrase for pre-publication tests

* Submodule update ##NateWr/i5045_publish_message##

* pkp/pkp-lib#5043 Add upgrade script to fix bad submission status

* pkp/pkp-lib#4857 Fix schedule for publication button and move locale string to shared library

* Submodule update ##NateWr/i4857_open_tab##

* pkp/pkp-lib#4873 Fix dependent file handling for versioning

- Deletes dependent files when galley deleted
- Checks if file is used by a previous version before deleting

* Submodule update ##NateWr/i4873_files##

* Update ru_RU locale after PR pkp#2478

* Update ru_RU locale after PR pkp#2481

* pkp/pkp-lib#4859 Fix searching with addition of versioning

* Submodule update

* pkp/pkp-lib#5098 Update workflow template with changes to data

* pkp/pkp-lib#5098 Update workflow handler with changes from pkp-lib

* pkp/pkp-lib#5098 Fix tests

* Submodule update ##NateWr/i5098_performance##

* pkp/pkp-lib#5044 Use locale string from shared library

* Submodule update ##NateWr/i5044_scheduled##

* pkp/pkp-lib#4861 Migrates cover images to support versioning

- Combines coverImage and coverImageAltText settings into one
- Migrates settings to publication_settings table
- Updates article landing page to show correct publication cover image

* Submodule update ##NateWr/i4861_covers##

* pkp/pkp-lib#5138 Fix capitalization of class name

* pkp/pkp-lib#5139 Fix custom block manager plugin; remove extraneous code

* Submodule update

* Submodule update

* pkp/pkp-lib#1375 Permit null issue Number and Year values

* Clean up PHP warnings

* Resolve count erroroneous call on DAOResultFactory

* pkp/pkp-lib#5122 Support Iterator pattern for DAOResultFactories

* pkp/pkp-lib#5122 Use Iterator pattern in search indexing

* pkp/pkp-lib#4880 Properly skip XML import test (to avoid missing publication database inconsistency from broken import code)

* pkp/pkp-lib#5122 Code review tweak

* pkp/pkp-lib#5122 Scrutinizer tweaks

* Submodule update

* Submodule update

* Submodule update

* pkp/pkp-lib#5122 Facilitate the use of Iterators in service classes

* pkp/pkp-lib#4859 Add metadata indexing to publication service

* pkp/immersion#25 Tweak for fr_FR abbreviation

* pkp/pkp-lib#4867 Initial work adding DOIs to publications

* pkp/pkp-lib#4867 Add DOI preview to publish form

* pkp/pkp-lib#4905 Restore support for exporting pub ids and metadata

* Submodule update ##NateWr/i4867_dois##

* Forward-port ae7c745 to master branch

* Submodule update

* pkp/pkp-lib#4804 Fix self-join problem on CrossRef upgrade SQL

* pkp/pkp-lib#4924 Properly display access status for pay-per-view purchases

* Fix filename typo

* pkp/pkp-lib#4593 Fix incorrect access indications in category listing

* Submodule update

* Update ru_RU locale after PR pkp#2496

* Submodule update

* Forward-port fr_CA emailTemplates.xml to master

* pkp/pkp-lib#4168 Migrate submission date_status_modified to last_activity

* pkp/pkp-lib#4168 Document new daysInactive API param

* Submodule update ##NateWr/i4168_last_activity##

* include two new email templates

* two new email templates

* two new email templates

* two new email templates

* two new email templates

* two new email templates

* two new email templates

* two new email templates

* pkp/pkp-lib#4867 Migrate DOI settings on upgrade

* Add issue querybuilder filter for issue ids to support browsebysection plugin

* Fix filter by section parameters in querybuilder

* Fix conditional check on empty array in issue query builder

* Submodule update

* pkp/pkp-lib#5122 Fix check for empty DAOResultIterator and change naming pattern

* pkp/pkp-lib#5122 Use Countable interface in DAOResultIterator

* Submodule update

* Fix PHP warnings/errors

* pkp/pkp-lib#5216 Update links to in-app help

* Add missing ID fetch

* updated fr_CA translation

* pkp/pkp-lib#5122 Update naming pattern when returning an iterator

* Submodule update ##NateWr/i5122_iterator##

* Submodule update: in-app help

* pkp/pkp-lib#5526 Add missing templates in other languages during upgrade

* pkp/pkp-lib#4705 Put code back to fix cover image issue

* pkp/pkp-lib#5208 Update URN plugin to support publications

- Adds FieldUrn component
- Makes the check number generation available in more places
- Updates plugin setting names

* pkp/pkp-lib#5208 Remove extra whitespace

* Submodule update ##NateWr/i5208_urn##

* pkp/pkp-lib#4705 Fixed spacing

* pkp/pkp-lib#4705 Removed spaces

* pkp/pkp-lib#5011 Forward-port to master

* Update ru_RU locale after PR pkp#2519

* Fix subject search

* pkp/pkp-lib#4684 Added mobile nav menu for smaller screens

* Replaced pkp_site_name in body.less to remove default 100% width

* pkp/pkp-lib#4684 HTML adjustments for default theme mobile nav menu

* pkp/pkp-lib#4684 Added close symbol to open mobile nav menu

* pkp/pkp-lib#4684 Renamed classes

* pkp/pkp-lib#4684 Toggle nav menu dropdowns on small/large screens

* pkp/pkp-lib#4684 Moved .pkp_nav_list class to a media query

* Moved .pkp_nav_list out of helpers, so deleting it

* pkp/pkp-lib#4684 Fixed mobile nav styles, added two search bars

* pkp/pkp-lib#4684 CSS for two menu bars WIP

* pkp/pkp-lib#4684 Fixed what is showing and what is not in search bars

* pkp/pkp-lib#4684 Styling mobile search bar

* pkp/pkp-lib#4684 Added separate search form for mobile

* pkp/pkp-lib#4684 Work out layout/design issues with mobile nav in default theme

* pkp/pkp-lib#4684 Remove unused search form template

* pkp/pkp-lib#4684 Improve style of task count in nav menus

* pkp/pkp-lib#4684 Fix inaccessible user nav item due to lost hover

* pkp/pkp-lib#4684 Fix mobile nav menu widths from phone to tablet size

* pkp/pkp-lib#4684 Remove mobile user nav styles on large screens

* pkp/pkp-lib#4684 Restore animated search styles in default theme header

* pkp/pkp-lib#4684 Fix search form class and mobile nav padding

* Submodule update ##NateWr/i4684_nav_menu##
ajnyga pushed a commit to ajnyga/ojs that referenced this issue Nov 17, 2019
ajnyga pushed a commit to ajnyga/ojs that referenced this issue Nov 17, 2019
ajnyga pushed a commit to ajnyga/ojs that referenced this issue Nov 17, 2019
ajnyga pushed a commit to ajnyga/ojs that referenced this issue Nov 17, 2019
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

No branches or pull requests

2 participants