Routing events between handlers in the JS framework

[NateWr]

This issue is a placeholder to track some work/experimentation to improve the way that our JS handlers can communicate between themselves. The suggested approach is to have a kind of global event router which is based on the observer pattern common in several JS frameworks.

I'd like to use this to document some of the lessons learned about how events in the JS Handlers (eg - UI "widgets) work.

Overview

As a general rule, events that are executed on a Handler do not bubble up like normal JS events. They're captured in an effort to ensure that Handlers remain encapsulated.

However, in some cases we have needed Handlers to talk to each other. This is done by white-listing certain events to be "published", using the publishEvent, trigger and triggerPublicEvent_ methods. These published events are triggered on the Handler's parent element in the DOM, from which they can bubble up.

This event is captured by a parent DOM element which has a handler attached, typically a complex Handler like a grid or the SiteHandler.

Because modals exist outside the normal DOM hierarchy, bubbling doesn't really get the job done. To accomodate this, there's a eventBridge option which can be passed to Handlers. This is used by modals to redirect events to the LinkAction which opened the modal.

The notifyUser event also gets it's own routing. Several handlers deliberately bubble the event up to the SiteHandler, which then makes a request to the server to decide what notifications to display.

So we have two ways for Handlers which are separate in the DOM (ie - not nested) to communicate with each other: an event that bubbles to SiteHandler and is then re-routed by SiteHandler, or an event bridge that reaches out to another element with a jQuery selector.

The maintenance problem

This architecture has worked to keep the Handlers highly re-usable. But it means that in cases where we do need Handlers to interact, including in cases where bubbling may not be sufficient, we have written a lot of code to reach out from one Handler to another.

The result has been that the connections between coupled Handlers tend to be ad-hoc, written into whatever method of the Handler happens to be doing that job right then. As a result, it's hard to have a clear sense of the dependencies between Handlers where they do exist, and to guard against breaking these when refactoring.

The (first) proposed solution

I looked into addressing this with the observer pattern, something I was familiar with from the Backbone.js framework. It would also give us an opportunity to start piecemeal refactoring towards a more common approach to JS-heavy applications.

The blocking problem with implementing the observer pattern

The observer pattern can’t be implemented because we lack a central registry of handlers. A handler which updates its content will remove large chunks of the DOM without tracking which handlers were caught up in the destruction. We will have references to listeners that are no longer present in the DOM (the lapsed listener problem).

The usual way to resolve this is with a central registry of handlers which deregister their events when removed (the dispose pattern). But because a handler doesn’t know what handlers are caught up in it’s refresh operations, it can’t trigger deregistration on embedded handlers (eg - a LinkActionHandler that's part of a GridHandler).

A new proposal

Without a significant refactor, we can’t have truly decoupled handlers. Instead, I’d suggest using a modified observer pattern in which the relationships between events and listeners are stored explicitly in the observer’s code. So handlers would not subscribe to events on the observer. Instead, the observer would have explicit instructions for passing specific events to specific handlers. The typical observer pattern looks like this:

Sender.init() {
    Observer.trigger(‘changeEvent’);
}
Listener.init() {
    Observer.listenTo(‘changeEvent’, Listener.respondToChangeEvent);
}

The Observer can be completely ignorant of the events that are passing through it. The modified observer pattern I’m proposing would nest lookup logic in the Observer.

Sender.init() {
    Observer.trigger(‘changeEvent’, eventData);
}
Listener.init() {
    this.bind(‘changeEvent’, Listener.respondToChangeEvent);
}
Observer.trigger(event, eventData) {
    if (event === ‘changeEvent’) {
        $(‘.listener’).trigger(‘changeEvent’, eventData);
    }
}

The Observer would not store references to handlers. Instead, it would look up any specific handlers by their associated DOM elements, performing a fresh lookup each time an event was triggered. This would provide decoupling of a sort, though the coupling would still have to be specified in a central Observer. It’s only marginally better than the current practice of triggering an event from one handler on another handler’s DOM element directly.

But is it really an improvement?

Relying on DOM lookups is not something I feel good about, to be honest. All this pattern really does is restructure the hacky DOM-based lookups away from the Handler into a central location (example). I think it will help (a little) with the maintenance issue by exposing these relationships more clearly. But the same could be done in theory with the existing eventBridge technique.

I don't see a way around these DOM lookup approaches without a pretty significant refactor of our Handlers. I've been wracking my brain but I haven't come up with anything.

One very simple refactor that I have made which I think is genuinely useful is that, on the PHP side, the JSONMessage object now supports multiple events. So you can register more than one event to return in a JSON response. Example: when publishing an issue you can return the dataChanged event that the grid is programmed to respond to, as well as a issuePublished event that can be broadcast separately.

The global event router, combined with multiple events in a JSONMessage, could be a good pattern to adopt more broadly (triggering notifications, tasks, etc from the PHP side). But I'm not sure it gets us very far down the road I originally had in mind of an event-driven UI with stronger decoupling of Handlers.

Examples

pkp-lib: https://github.com/NateWr/pkp-lib/commits/i2163_event_router ojs: https://github.com/NateWr/ojs/tree/i2163_event_router

[asmecher]

Thanks, @NateWr, good reading.

Do you remember when you first started working with the PKP JS code that you proposed a change to the handler lifecycle? IIRC you wanted static calling or something. Would that be a useful thing to revisit in mangling an observer pattern into our DOM usage?

[NateWr]

I dug up my old doc on that when I was looking into this. I'm not sure it would solve the problems we're facing in this case, although I think questions about how we initialize components will keep coming up if we start to introduce parts of an existing framework.

I've come up with an ad-hoc way to get a sort-of registry of handlers. Basically, we have every handler crawl up the DOM hierarchy and register itself with the first handler it finds. So each handler gets a reference to handlers attached to elements within the DOM.

This works because in the majority of cases we do full-handler refreshes all at once. So when refreshing, a handler can loop through it's child handlers and de-register event listeners.

There are a few cases where we do partial refreshes and I'm working on those at the moment. Some I'm refactoring away. Others (like replacing single grid elements) I'll probably write something special to handle those cases. But it's a pretty small group of cases (maybe half a dozen) which I've been able to identify.

I'm hoping this will be pretty reliable, though I'll have to see once it's in place if we end up with memory leaks.

[asmecher]

@NateWr, thanks, that sounds like a decent short-term plan.

[NateWr]

I've managed to get a global event router working alongside our current JS system. I've issued a couple of dummy PRs in my own repos, to demonstrate more concretely some of the refactoring that's been involved. Although I suspect we'll go in a different direction with our JS framework, much of the refactoring that's been done will still be important.

The PRs with the code changes: https://github.com/NateWr/pkp-lib/pull/1/files https://github.com/NateWr/ojs/pull/1/files

A run-down of what's been changed:

• Added Underscore and Backbone libraries for some utility functions and objects.
• Refactored JSONMessage (1) and Handler.js (2) to support sending and receiving multiple events.
• Created JSONMessage->setGlobalEvent() to send an event intended for the global eventRouter. It adds a data attribute of isGlobalEvent to the eventData.
• Set up $.pkp.classes.EventRouter for registering event listeners and triggering events globally.
• Used global events to sync the back and future issue grids when an issue is published/unpublished.
• Used global events to refactor the user merging functionality.
• Refactored InfoCenter to remove the need for partial refreshes.
• Refactored handlers to register with their closest parent handler. Created functions for unregistering global event listeners when a handler is removed (including child handlers). Refactored all handler HTML replacement code to use wrappers, to ensure that global events are unbound from handlers before they are removed (1, 2, 3, 4.

This last bit entailed the most arduous refactor. I think it will be key no matter what framework or approach we adopt, to ensure that listener references can be cleaned up as our handlers refresh HTML. I tried to make sure I got everything except for really minor replacements that wouldn't contain handlers, but I'm sure I've missed something.

@asmecher I'm hoping you'll be able to give this a once over and see if you spot anything that looks potentially problematic. Although I suspect we won't go with the specific Backbone Events approach here, much of the work of refactoring the way that Handlers modify HTML will be a pre-requisite for almost any approach we take (as far as I'm aware).

PS - where you see _.() functions, that's the Underscore.js library, a collection of helper functions and a pre-requisite of the Backbone library.

[NateWr]

I've made an initial set of PRs so that we can begin discussing some of the architectural choices involved in integrating Vue.js.

PRs: https://github.com/pkp/pkp-lib/pull/2339 https://github.com/pkp/ojs/pull/1299

What's here:

• Implements webpack 2 to manage JS dependencies and compile single-file Vue.js components. That means we have Babel running and we can write ES6. I haven't used it much, though, as I'm not too familiar with it yet.
• Adds a global event router (a Vue instance). This is used to communicate between components and bridge interactions between the legacy JS framework and the new stuff. In this example, you can see an old PHP handler passing a global event to a ModalHandler in the old JS framework, where all handlers now know how to fire global events on the new eventBus. And then a handler, whether part of the old framework or a new Vue.js component, can listen for global events. (Where Vue.js components handle the interaction themselves, they should trigger global events directly instead of having this done in the PHP handler. The JSONMessage->addGlobalEvent() method is a temporary solution to manage the transition.)
• The submissions lists have been replaced with new components powered by Vue.js. -- PHP: A SubmissionListHandler has been created. The handler is responsible for providing basic config information (eg - title, translation strings), fetching the list data, and processing commands on the server side. -- Vue.js: A SubmissionsListPanel Vue handler, which makes use of various sub-components. -- The handler is then instantiated and config data is passed to the template, where it's loaded inside a wrapper with a small initialization helper.

As part of implementing the new submission lists, I've made a number of other changes:

• I needed a way to compile a representation of a submission for passing to the frontend, so I created a Submission::toArray() method here. I suspect Alec may have some ideas for how he wants to handle this better. It's not terribly performant. But this is a good example of an area where there's lots of potential overlap with a REST API. I put a couple ideas in there for how the data model could be extended by plugins.
• I did a lot of refactoring for how review stage and review assignment statuses are handled, stripping out lots of task handling and trying to focus on more of a singular status approach. There's room for improvement here (for instance, review assignments have no status in the database so it always has to be calculated from other bits of data). But it did clear out a few lingering refresh bugs. I'd like to do similar work for the other stages. And regardless, we'll need a more consistent approach to make some of the backend data like status globals available on the frontend. They're stitched together poorly right now. One approach is how I test current user access roles by loading all role globals and creating a helper function that we can use in components. Centralizing some of these needs will be good as we incorporate more components.
• Each component needs to pass the required translation strings in it's configuration params. Then these need to be passed to sub-components. Then we can perform data insertions with a helper function attached to every Vue instance/component. In retrospect, I think we should standardize the keys, so that if a locale key is submission.list.count, that should be the key passed to the i18n object hash on the frontend. And all translation strings in Vue components should be run through the __() function, even if they don't need data injection. This will entail a minor performance hit and lead to a bit more verbose key names, but I think it will help us keep track of the links between the frontend and the XML locale lists.
• With these revisions, authors can no longer delete their own (uncompleted) submissions. I've structured the broad role access restrictions to specific handler operations in a way that can be surfaced and accounted for on the frontend. But this only works for blocking a whole role on an operation (I use the term "route" in the component config). In cases like the delete operation, access for authors depends on whether they're an author too. That can be accounted for by checking stage participants against the current user. But I think we might benefit from finding a way to surface access restrictions within the route config that gets passed to the handler. We'll save ourselves some frontend logic and hopefully get a single source of truth for access control.

How to get it working

1. Install Node. It will come with npm (Node Package Manager), which is like Composer for the JS community.
2. After checking out the branch, go to the OJS root directory and run npm install.
3. Once that's done, you can run npm run dev. Webpack will compile the Vue.js components in a dev build, and it will wait and recompile automatically whenever you change a file. Run npm run build-dev to build once without watching. And npm run build to build the JS package for production.

There's plenty more to talk about, but hopefully this gives you a high-level overview of what's going on. Enough to look at the PRs and think about how things might be structured differently, or where pitfalls might occur.

[asmecher]

Woo, this looks great. It's also going to involve a lot of learning on my part, but I can see huge potential already and I'm really pleased by the signs I can see of a possible migration process that won't impose a big-bang.

I have some line-by-line suggestions but I'll keep them to myself until you're ready for a code review at that level.

I do think it's worth working with the Slim microframework (or something similar, but Slim shares concepts with what you've written) rather than ListHandler. Have a gander at https://github.com/asmecher/pkp-lib/commit/e54010a7b7718004685b9fb6ceeb986d5ba1d83f. As you've noted, there's overlap with the API, so a unified approach would pay off handily, I think.

I haven't set up and run this yet -- when you're ready for the rest of the team to dive into it, maybe we could start with a quick walk-through during one of the calls? And of course some of the TC folks would love to have a look.

[NateWr]

Yeah I'd love to get it tied in with the REST API. Can you give me a quick example based on your branch of how I would create a new route and authenticate a request to it?

[asmecher]

This is probably a good time to get @kaschioudi involved, since he'll be working with the API stuff. Kassim, could you dust off that work and make sure it's still functioning with the current master branch, and send Nate a link to an example route?

[kaschioudi]

sure. @NateWr , I will get back to you shortly.

[kaschioudi]

@asmecher : I am unable to locate APIProfileForm class from the work we did during the sprint ((cf. https://github.com/pkp/pkp-lib/compare/master...asmecher:api#diff-7038e7a456f6b34aa77c9cb9d813e0c3R223).

ps: I am working with https://github.com/pkp/ojs/compare/master...asmecher:api_work and https://github.com/pkp/pkp-lib/compare/master...asmecher:api

[asmecher]

Hmm, I'm afraid I can't find it either. It would've been a subclass of BaseProfileForm with a single field implemented -- the API key. The template had a button to generate a new API key when it was clicked. That's all that was there. You could start with something like lib/pkp/classes/user/form/PublicProfileForm.inc.php and adapt it from there.

[kaschioudi]

Working code available here: https://github.com/kaschioudi/ojs/tree/rest_api https://github.com/kaschioudi/pkp-lib/tree/rest_api

@NateWr : two endpoints are currently available: /{contextPath}/api/v1/submissions/{submissionId} /{contextPath}/api/{v1/submissions/{submissionId}/files/{fileId}

First endpoint is working whilst second endpoint is currently broken. If I remember correctly, it's failing because the authorized context requires additional parameters in the URL. (cf. https://github.com/kaschioudi/ojs/blob/rest_api/api/v1/submissions/SubmissionHandler.inc.php#L84) @asmecher

[NateWr]

Thanks @kaschioudi. I'm running into a permissions issue when I make a request to //{contextPath}/api/v1/submissions/{submissionId}. It seems to be hitting this:

https://github.com/pkp/pkp-lib/blob/master/classes/handler/PKPHandler.inc.php#L279

And then returning a false decision:

https://github.com/pkp/pkp-lib/blob/master/classes/handler/PKPHandler.inc.php#L287

I haven't really been able to figure out how our authentication system works (decision managers, policies and so-on), so I'm not really sure what's going on here. The comment suggests that the URL needs to be white-listed...

[kaschioudi]

@NateWr : are you sending the api key? You can create the api key from user profile page. Then when calling from javascript, you need to send that key as apiToken query parameter.
/{contextPath}/api/v1/submissions/{submissionId}?apiToken=xxxxx

fyi. I was able to get the second endpoint to work. However the signature is a little different now because SubmissionFileAccessPolicy expects fileId and revision as query parameters. /{contextPath}/api/{v1/submissions/{submissionId}/files?fileId=x&revision=y (note that revision is not required)

you can get the latest code from rest_api branches.

[NateWr]

Thanks @kaschioudi I'll look into it. I assume for backend usage we want to generate an API key automatically?

[NateWr]

Yeah that was an off-hand comment. On second thought, for internal requests within the app, we probably just want to use the cookie-based session authentication we're already using. These get passed along with the ajax requests, and it's easy to attach a CSRF token to the header with every request. This should suffice for authentication from within the PKP app, right? Any idea on how to go about this?

[kaschioudi]

Cookie based session authentication should work. apiKey is provided as an alternative. I thought you were trying to hit the endpoint from command line.

While logged in, when you open the api endpoint URL directly in the browser, does it work or you get 404?

[NateWr]

With just cookie details, I'm getting a 404. I haven't really looked into it though. I've just been short-circuiting the authorization for now to try out some stuff.

I've managed to build out a /submissions endpoint that will work for the submissions lists. But I'm stuck trying to figure out how to ping that endpoint directly from another place in the code, so that we can gather an initial list of submissions to deliver with the initial HTML. So I want to get the results of an endpoint without sending an HTTP request.

Slim seems to have a concept of Subrequests, but I've been unable to get it to work. And I found a 4.0 roadmap that suggests he's planning to ditch them entirely.

]I've tried bootstrapping the SubmissionHandler and calling a get request there, but the $request doesn't match and I was unable to get it to process an endpoint. I then tried going all the way back to the APIRouter, instantiating that. But I wasn't really able to get the bootstrap up properly, and it seems like a really bad idea to essentially bootstrap the entire app a second time.

Am I missing something obvious here? It seems like it would be a really normal thing to want to consume an API endpoint without necessarily making a separate HTTP request. But the Subrequest feature of Slim is almost completely undocumented and I can't seem to find any discussion of this anywhere.

We can get around this by building a service layer that the API and other code can consume equally. But I'd like to avoid that if possible.

Any ideas?

[kaschioudi]

unless OJS core provides a way to have sub requests, I can't really think of anything else other than moving the logic to a service layer. I believe such approach is the correct way to do what you described otherwise you would introduce some coupling between your code and the endpoint.

[NateWr]

Ok, I've now got a branch where the new submissions list is working on top of the new REST API endpoint for /submissions. Some PRs for an easier dive into the new code:

https://github.com/pkp/pkp-lib/pull/2399 https://github.com/pkp/ojs/pull/1337

This is probably the last I'll be able to work on it before I go away on holiday (back April 19). If the Services model that @kaschioudi proposed goes forward, hopefully we can use this branch as a base (the rebasing is getting a bit painful now).

Notes/Discussion:

• ConfirmationModal now accepts callback and callbackArgs params, which it will fire when confirmed. I ended up not using this, but I think it will be useful as we integrate things further down the road.
• I'm trying out BEM naming syntax for class names. It's always felt a bit verbose to me but it has kind of become an industry standard.
• I added an Unassigned tab, because I couldn't think of a reasonable way to combine those into the API-based request structure. I think that's fine, but maybe others will complain. Only Journal Managers and Admins will see it. If we keep it, we need to add an entry in the contextual help panel.
• Do we need to be passing and validating CSRF tokens with API requests (eg - DELETE requests)?
• Request::getUser() is called statically in several places, but it uses $this in it's API check. This will need to be worked out at some point.
• If a Journal Manager makes a submission, they still get access to the full workflow (not the author dashboard) for that submission. I assume that's ok and mirrors the existing setup. Is that alright?
• ReviewAssignment had the cancelled and replaced statuses removed, but there are still some remnants around (like an Email Template) and some stuff calling REVIEW_CANCEL_. This is related to changes in #2173 (cc @bozana).
• I never got the authorization working properly in the API's SubmissionHandler. I've jsut been short-circuiting it but I think @kaschioudi has a handle on that side of things. I'd appreciate a review of that for the new delete handler.

I think the Reviewer's list of submissions could use some work to be more relevant to them (they don't need stage info or any of that other stuff). Here's a list of things I noted for later:

• Confirmed reviews should be moved to their Archives list.
• Reviews could be ordered by nearest deadline.
• We could add a “next deadline: response due on X” notice for reviewers. Or just a “next action” notice of some kind. “You’ve been asked to review this submission. Please confirm your availability by X”. “You’ve accepted this review. Please submit the review by X.” “Thanks for submitting your review. You recommended X. You’ll be notified when an editor has confirmed it.”
• We should provide some positive visual feedback when response and reviews are submitted, to give them a sense of accomplishment. In archives, we could display the reviewer’s recommendation, and let them know when a submission has been published: “Published in X”
• Should reviewers be able to view the content of their previous reviews?

(Probably lots of other things but that's what I've got in my notes. :) )

cc @asmecher

[NateWr]

Oh yes, one more thing:

• I've left all the submissions list grid handlers in place now. I believe we need to convert the exportable submissions list grid, but we can then remove them all.

[asmecher]

Excellent, @NateWr!

Do we need to be passing and validating CSRF tokens with API requests (eg - DELETE requests)?

OTOH: yes, for cookie-based authentication, where CSRF attacks are a risk. No, for API requests that include some other kind of auth token.

If a Journal Manager makes a submission, they still get access to the full workflow (not the author dashboard) for that submission. I assume that's ok and mirrors the existing setup. Is that alright?

Yes, at least for now -- we will eventually have to consider how to treat mixed roles, e.g. Editor and Reviewer, or Editor and Author.

@NateWr, when you're back, let's schedule an orientation for the team before breaking it out into a more detailed code review etc. There will be at least one more (probably irritating) rebase after you return, but hopefully not more than that.

[marcbria]

I'm trying out BEM naming syntax for class names. It's always felt a bit verbose to me but it has kind of become an industry standard.

I full agree with the decission, although the verbose abuse. As usual, this work looks very promising @NateWr. Thanks.

[kaschioudi]

I never got the authorization working properly in the API's SubmissionHandler.

@NateWr I have looked into this and authorization is not working because SubmissionHandler uses SubmissionAccessPolicy (https://github.com/NateWr/ojs/blob/i2163_js_with_api/api/v1/submissions/SubmissionHandler.inc.php#L80) which depends on SubmissionRequiredPolicy, meaning that a submissionId needs to be provided (https://github.com/NateWr/ojs/blob/i2163_js_with_api/api/v1/submissions/SubmissionHandler.inc.php#L91)

Basically your endpoint /submissions cannot work with the current implementation of SubmissionHandler.

[asmecher]

The authorization stuff came up briefly at https://github.com/pkp/pkp-lib/issues/1503 already -- we already have a few cases where IDs are supplied to policies in different ways (watch for e.g. $submissionParameterName), so I think it makes sense to survey these, check which are still required (I suspect some of these have since been refactored into obsolescence), and make that aspect more flexible. @kaschioudi, during the sprint I started tinkering with a getEntityId function so that auth policies could delegate the ID fetch back to handlers -- see e.g. https://github.com/pkp/ojs/compare/master...asmecher:api_work#diff-083a379306d6670d93b958b44bd05b0fR58. I don't love this solution and it could definitely use more work.

[NateWr]

Ok, another update.

PRs: https://github.com/pkp/pkp-lib/pull/2399 https://github.com/pkp/ojs/pull/1337 https://github.com/pkp/ojs-user-guide/pull/12

The big picture changes:

• Uses @kaschioudi's services layer to share code between REST API and UI component controllers. Example
• Uses the Laravel database query builder integrated by @kaschioudi. Example
• Introduce a separate backend API for internal use. Example
• The Submissions List component now uses the backend API. (There are some lingering bugs.)

I think now is a time to get a preliminary code review, @asmecher. At the very least, I'd like to know how you feel about some of the architectural choices (services layer, query builder, backend api, vue.js setup) before going further. I've pulled some of @kaschioudi's changes in ad-hoc, so the longer this lingers the more painful it will be to merge down the road.

In short, I'd like to move quickly (and carefully) towards merge. With that in mind, can we schedule a call this week to provide an orientation to the code?

[asmecher]

@NateWr and @kaschioudi, I'm still not sold on the use of a new database layer here. It looks like our use of it is still pretty lightweight, i.e. the code with the query builder is not so different from our usual approach of hand-building SQL (though it's possible I'm missing the motivating case). The illuminate/database package requires PHP 5.6.4, while our system requirements are currently PHP 5.3.7. And my longer-term plans are to scrap ADODB and replace it with something else; I'm not sure Laravel is going to meet our needs (particularly around schema management, which is probably a defining requirement), meaning we might need to either remove Laravel or run two frameworks at once.

[kaschioudi]

@asmecher I am not using the whole Laravel framework but only the illuminate/database which is a database toolkit submodule.

The intent here is to use illuminate/database as a helper library to build SQL queries in a more elegant way by taking advantage of its expressive query builder. The library is compatible with PostgreSQL and MySql so that also takes away part of the pain of writing queries compatible with both RDBMS. However I still use AdoDB as database layer (cf https://github.com/kaschioudi/pkp-lib/blob/api_services_integration/classes/services/SubmissionService.php#L72-L76)

I am using illuminate/database 4.1.* (cf https://github.com/kaschioudi/pkp-lib/blob/api_services_integration/composer.json#L16) which requires PHP 5.3 (cf https://packagist.org/packages/illuminate/database#v4.1.30)

[asmecher]

OK, that resolves the version number issue, and if it looks worthwhile we could consider bumping the PHP requirements for our apps further.

I'm still not sure about the motivation for introducing this right now. I don't mind the idea of a query builder, but the defining feature for choosing an ADODB replacement (and we need one, and hopefully not more than one) is the schema management toolset, necessary for installing OJS and managing schema changes on upgrade. Laravel does have something of a toolset for this -- see https://laravel.com/docs/5.4/migrations -- but I'd like us to survey a few options before picking one.

[kaschioudi]

I did not introduce illuminate/database as a replacement to ADODB and that's the reason why I don't use it to interact directly with the database.

I am actually using that library as a tool to easily construct complex queries without the need to deal with raw SQL directly. At the moment, it's only used to build SubmissionListQueryBuilder (https://github.com/kaschioudi/pkp-lib/blob/api_services_integration/classes/services/queryBuilders/SubmissionListQueryBuilder.php) in order to provide a simple interface for client code (see https://github.com/kaschioudi/pkp-lib/blob/api_services_integration/classes/services/SubmissionService.php#L62-L67).

if this approach does not seem right to you, please let me know and I will update the code.

[NateWr]

@asmecher I'm not too invested in a query builder and I'm happy going whichever way we decide. But I kind of nudged @kaschioudi down this path so I thought I'd run through how it came about.

SQL is a weak point for me. When I started refactoring the submission list, and interacting with the SQL statements, I had some trouble keeping the params and statements straight and in order:

https://github.com/NateWr/pkp-lib/commit/1059de7b21974061fc3fe26cb1866ec241dfaf5a#diff-984175ac8c5e5a2aef5436001a896f6eR751

I found I was duplicating some of this code:

https://github.com/NateWr/pkp-lib/commit/52c57f18ffe215f30a9cd9c308f1fa2bd6db9876#diff-984175ac8c5e5a2aef5436001a896f6eR687

When the REST API came in, I spied an opportunity for a more generic getter for the submissions DAO, which would allow us to maintain one SQL instead of a bunch of different ones. But assembling all of the SQL with a lot of contingent parameters was causing me a lot of grief (remember: not great at SQL here). So I built a little query builder to get my brain straight on it:

https://github.com/NateWr/pkp-lib/commit/58c67eebd674e7a4557c33198ce3798b65ae9970#diff-af4e4abe74f63f90aa12d3cf11dedba2R1

I then used that in a more generic getter method:

https://github.com/NateWr/pkp-lib/commit/58c67eebd674e7a4557c33198ce3798b65ae9970#diff-984175ac8c5e5a2aef5436001a896f6eR322

The main benefit for me was the ability to add statements to the query out of order, which helped me group the code according to the $params being passed in:

https://github.com/NateWr/pkp-lib/commit/58c67eebd674e7a4557c33198ce3798b65ae9970#diff-984175ac8c5e5a2aef5436001a896f6eR379

@kaschioudi then took what I had hand-rolled and implemented it on a standardized query builder lib (Illuminate) and I then used that approach in my branch:

https://github.com/NateWr/pkp-lib/commit/5f96f45f1f34a26c077fe31723e4c5fc697be710#diff-da82651038038bf9e89fd1b35feb69a3R116

The Illuminate approach took me a bit of getting used to, to figure out how and when to use the Capsule::raw() thing, and how to build some of the complex query relationships that were needed. But I got there.

And in at least one place, it's been useful. For that DB issue I was having where I wanted to use a specific offset but still return the total count of results. I ended up needing to use a second query. And in this case, I was able to re-use all of the code setting out the conditions, and just pass a flag to issue a separate select command:

https://github.com/NateWr/pkp-lib/commit/50ca6e28de42d459ee49daadf10ad292851db5a8#diff-da82651038038bf9e89fd1b35feb69a3R117

As I said, I'm not really one to decide whether using a query builder is worth the extra level of abstraction or not. You guys are going to be maintaining most of this code and I only dip in for rare instances. But at least this provides a history as to how the decision came about and how it's used in this commit. Hopefully that helps you make a decision.

:+1:

[asmecher]

Thanks, @NateWr and @kaschioudi. This all makes sense, and the tool you've chosen looks fine; I just wasn't planning on grappling with ADODB replacement toolsets yet. I suppose now is as good a time as any to do some research/planning. I've filed this over at https://github.com/pkp/pkp-lib/issues/2493 for consideration. I don't want to hold either of you up, so please continue, and I'll let you know if we run into any showstoppers.

[marcbria]

Long shot here, but let me point that some CMS moved to generic frameworks that (for instance) include an abstraction layer over multiple DBs. I think the discussion is between Laravel, Symphony, Yii... https://opensource.com/business/16/6/which-php-framework-right-you

Again, moving to "the facto" standars will help one day to get more contributions from developers that don't know PKP's API but play well with (for example) Laravel + vue.js + bootstrap.

[NateWr]

I've now got PRs for all three repos. I imagine I'll be fighting with the tests for a bit, but getting close to ready:

https://github.com/pkp/pkp-lib/pull/2399 https://github.com/pkp/ojs/pull/1337 https://github.com/pkp/omp/pull/412

[asmecher]

Yeow, let's get this merged so the rebase sword of Damocles is no longer threatening you. How synced is this with the API work?

[NateWr]

@asmecher Ok, some progress on the tests. Still some lingering issues:

• The selenium tests are failing right out of the gate. They can't find the OJS Installation text. I fixed one issue with this, but I'm not not able to duplicate. I follow the install instructions and I'm able to get to the OJS install screen (and successfully install). Can someone else test? Maybe it's something specific to PHP < 7.0?
• PHP 7.0 is going to have issues with the current buidl process, probably, because mysql is no longer a valid driver. The config file needs to be modified to use the mysqli driver. How can I do this?
• The node_modules directory (where npm puts the dependencies) is causing havoc. Any idea what's going on here and how to exclude it from checking the node_modules dir? https://travis-ci.org/pkp/ojs/jobs/238416023

[asmecher]

PHP 7.0 is going to have issues with the current buidl process, probably, because mysql is no longer a valid driver. The config file needs to be modified to use the mysqli driver. How can I do this?

The shell scripts that run the tests determine what the test environment is like (e.g. whether it's a MySQL or PostgreSQL build, etc.) and pass those details to the Selenium/PHPUnit test that performs the installation. It should be ready for PHP7.0 without changes (and is already working for PHP7.1).

[asmecher]

The node_modules directory (where npm puts the dependencies) is causing havoc. Any idea what's going on here and how to exclude it from checking the node_modules dir?

Yup, this is the XML linting script (lib/pkp/tools/travis/validate-xml.sh). It checks a blacklist of exclusions (tools/xmllint-exclusions.txt) and otherwise checks every XML file it can find in the installation. I'd suggest adding the node_modules to the exclusion list.

[asmecher]

The selenium tests are failing right out of the gate.

Looking at https://travis-ci.org/pkp/ojs/jobs/238416025, the build scripts dump out a bunch of potentially useful debugging information at the end of the run. Spinning out line 855 ($ cat error.log), I see:

[Thu Jun 01 16:45:47 2017] [error] [client 127.0.0.1] FastCGI: server "/home/travis/build/pkp/ojs/cgi-bin/php.fcgi" stderr: PHP Parse error:  syntax error, unexpected '[' in /home/travis/build/pkp/ojs/lib/pkp/lib/vendor/nikic/fast-route/src/functions.php on line 12, referer: http://localhost:4444/selenium-server/core/Blank.html?start=true
[Thu Jun 01 16:45:47 2017] [error] [client 127.0.0.1] FastCGI: server "/home/travis/build/pkp/ojs/cgi-bin/php.fcgi" stderr: PHP Parse error:  syntax error, unexpected '[' in /home/travis/build/pkp/ojs/lib/pkp/lib/vendor/nikic/fast-route/src/functions.php on line 12, referer: http://localhost/

[NateWr]

Thanks for the tip on the xml linter and yes, of course the PHP 7 testing is already set up. Not sure what I was thinking.

So the syntax error is in a dependency of Slim API. It's choking on the [] array syntax in FastRoute here:

https://github.com/nikic/FastRoute/blob/master/src/functions.php#L12

This seems to have been added in PHP 5.4, so I'm not sure what the issue is.:

http://php.net/manual/en/migration54.new-features.php

I found mention of a similar problem, but in that case the person was mistakenly running PHP 5.3:

https://github.com/nikic/FastRoute/issues/119

@asmecher Is it possible that the PHP version is getting reset, or that FCGI is not configured to it or something? I'm a but out of my depth here.

[asmecher]

Ah, interesting. I'm using a FastCGI config of my own demisedesign back in 2014 -- see https://github.com/asmecher/travis-suexec-php (fixed that -AS) -- and I'm configuring PHP execution through /usr/bin/php5-cgi. I wonder if there's a different php7-cgi or something. Will check. If so, that would mean we're running unit tests etc. via PHP7, but potentially executing the app via PHP5.

UPDATE: I'm working on this over at https://github.com/asmecher/ojs/tree/fix-fcgi and https://github.com/asmecher/pkp-lib/tree/fix-fcgi

[NateWr]

@asmecher I'm still battling the JS linting tests. I've come across a series of error messages and I can't figure out how to resolve them.

1. Undefined globals: _ (the Underscore.js lib) and pkp (a new global for handling some Vue stuff, deliberately kept separate from $.pkp so it's no jQuery dependent). I've added them to the closure externs, but the linter is still complaining that they're undefined.

2. Wrong line breaks. The linter is complaining about having three lines before a constructor. But those files do have three lines (here and here).

3. The linter asks for a string here, but $.html() does return a string.

4. I'm struggling to figure out where and how several vars should be documented to please the linter.

[asmecher]

Wrong line breaks. The linter is complaining about having three lines before a constructor. But those files do have three lines (here and here).

There's a bit of preprocessing that goes on (in lib/pkp/tools/buildjs.sh IIRC). I think you have to actually use two; check other files for examples.

[asmecher]

The linter asks for a string here, but $.html() does return a string.

jQuery externs are defined in lib/pkp/tools/jquery-externs.js -- that declaration might need correcting.

[asmecher]

I'm struggling to figure out where and how several vars should be documented to please the linter.

Welcome to Crockfordland. See e.g. this type annotation. I remember having trouble finding a good reference on exactly what was allowed/expected.

[asmecher]

@NateWr, on https://github.com/pkp/pkp-lib/issues/2163#issuecomment-306239392 -- I found a fix for the underlying tools, but the repeated deprecations warnings/errors during the build process are a menace that blow the log file size out past Travis' 4MB limit. Fixing this will require a lot of monkeying around, unfortunately, and I don't want to do that while we have some major merges on the horizon. It would probably be nasty to keep them synchronized.

Proposal: See if you can get your code working with the PHP5.6 tests, as that uses the right php-cgi binary and meets our PHP minimum requirements. Then we can merge that, and possibly the versioning work @bozana has been stewarding, and possibly a minimum of the subscription work I've been doing (which already contains a little bit of said monkeying). Then I can spend some time on fixing all the deprecation issues that are causing warnings. A major one of those will be static use of Request methods, which has long been deprecated and would be nice to finally fix.

[NateWr]

@asmecher Are you sure about the PHP 5.6 tests? I'm getting the same syntax error messages on those tests, suggesting the older php version is running there too: https://travis-ci.org/pkp/ojs/jobs/239924214#L868

[Tue Jun 06 11:26:43 2017] [error] [client 127.0.0.1] FastCGI: server "/home/travis/build/pkp/ojs/cgi-bin/php.fcgi" stderr: PHP Parse error:  syntax error, unexpected '[' in /home/travis/build/pkp/ojs/lib/pkp/lib/vendor/nikic/fast-route/src/functions.php on line 12, referer: http://localhost:4444/selenium-server/core/Blank.html?start=true
[Tue Jun 06 11:26:43 2017] [error] [client 127.0.0.1] FastCGI: server "/home/travis/build/pkp/ojs/cgi-bin/php.fcgi" stderr: PHP Parse error:  syntax error, unexpected '[' in /home/travis/build/pkp/ojs/lib/pkp/lib/vendor/nikic/fast-route/src/functions.php on line 12, referer: http://localhost/
[Tue Jun 06 11:26:43 2017] [error] [client 127.0.0.1] FastCGI: server "/home/travis/build/pkp/ojs/cgi-bin/php.fcgi" stderr: PHP Parse error:  syntax error, unexpected '[' in /home/travis/build/pkp/ojs/lib/pkp/lib/vendor/nikic/fast-route/src/functions.php on line 12 

[asmecher]

I'm still tinkering with this -- let me see what I can do.

[NateWr]

Ok, sorry, still have a few JS linting errors I'm just not able to resolve:

1. Still getting the issue with globals. As far as I can tell, these are defined in the externs just like other globals.

2. It's telling me a property isn't defined on an object. But other properties on that object aren't defined too, and they're not throwing any errors.

3. It's complaining about a type annotation for a handler's options. But I can't see any difference with other keys in the object which aren't causing errors.

[asmecher]

@NateWr, one quick note on Travis testing -- it turns out that PHP7.0 tests won't work on the master branch either at the moment, so I'd suggest removing that in favour of just PHP5.6 and PHP7.1. It seems that PHP7.0 was a little stricter about enforcing constructor syntax, and PHP7.1 has relaxed that again for the moment. Meanwhile, I'll continue to work on PHP5.6 and PHP7.1 with the correct php-cgi binary and will report back here.

(We could probably get PHP7.0 testing going by selectively using this less-than-ideal config for PHP7.0 only.)