Add the Request object to the constructor for Components

[ctgraham]

From #1923 (comment):

[W]e should be providing the $request object more consistently to the controller classes, including GridCellProviders etc., perhaps by passing them in right when those classes are instantiated and having them save a reference. Right now access by those objects to the instantiated $request is spotty.

This is a blocker for #1923.

[crism]

Visiting .../ojs_dev/index/admin/index:

Call to undefined method Request::getId() in .../ojs_dev/lib/pkp/classes/controllers/grid/GridCellProvider.inc.php on line 50

[crism]

Looks like GridCellProvider::render() expects $row as the first argument (and getId() is called on it), but we’re now passing a request.

[ctgraham]

Good catch. That will be a missing change in the calling function. $request should be in the constructor now, and not in the render() arguments.

[crism]

Ah-hah… you rewrote GridCellProvider::render(), but there’s a call somewhere that didn’t catch up. I don’t have a stack trace, though… to the grepmobile!

[crism]

GridCategoryRowCellProvider::render() and GridHandler::_renderCellInternally() are culprits… shall I fix them, or will you?

[ctgraham]

You're welcome to fix and PR into ulsdevteam's refactor-gridcell-constructor branch, if you've already got them. Otherwise, I'm almost done resetting my dev environment to be ready to start into fixes myself.

[crism]

Let me finish tracking this further upstream and see where you stand…

[crism]

Ugh… it’s a little more global than just the calls to render(). GridCategoryCellProvider has subclasses, many of which are explicitly passed a request.

[crism]

Progress, though; with the latest commit, at .../ojs_dev/index/admin/index:

Call to undefined method GridRow::getCellActions() in .../ojs_dev/lib/pkp/classes/controllers/grid/GridCellProvider.inc.php on line 101

[ctgraham]

Where is GridCategoryCellProvider defined? I'm not seeing it.

[crism]

It’s GridCategoryRowCellProvider, in classes/controllers/grid. Sorry for the typo. But I meant GridCellProvider, anyway, that has subclasses.

[ctgraham]

I think the next step (for next week) is to identify whether GridRows and GridColumns should all be constructed with a $request passed in, per errors like:

PHP Warning:  Missing argument 1 for GridCellProvider::__construct(), called in ./lib/pkp/classes/controllers/grid/GridCategoryRow.inc.php on line 31

[crism]

Yeah, I think the refactoring needs to be more thorough. It’s yak-shaving time! I may work on this this afternoon, if you’re done for the week.

[crism]

Found a few lacunae to check in.

Also dependencies in other repos:

• pkp/staticPages
  • StaticPageGridCellProvider
  • StaticPageGridHandler
• pkp/translator
  • LocaleFileListbuilderGridCellProvider
  • LocaleFileListbuilderHandler
  • LocaleGridHandler

[ctgraham]

@crism, with your commits, I'm no longer seeing failures in the error log. Do you see any additional?

@asmecher, I'm particularly curious about cases like:
https://github.com/ulsdevteam/pkp-lib/blob/015c38da97d2b90778a7de73237ab13e306aacbe/classes/controllers/grid/CategoryGridHandler.inc.php#L41-L45
Here we now require an instantiated request object in the constructor, when it clearly appears that there is a two-step ::construct() then ::initialize() process. It makes me think we should be deferring the assignment of the members like GridBodyElements to the initialize() method instead. Is that legitimate? Do you have any sense of how disruptive that will be?

[crism]

Looked through a few grids, including plugins and their settings, submissions in various states, and saw nothing in the error logs. Looks clean to me.

[crism]

I’ve also attempted to audit all the uses of GridCellProvider and its subclasses, and they all seem to be clean now.

[asmecher]

@ctgraham, I'm not particularly concerned about where the $request object gets passed in -- perhaps it's more useful to consider where it's typically used. The __construct/initialize split doesn't exist through most of the stack (just e.g. in Handlers) and I wouldn't want to replicate that all the way down the stack into e.g. GridBodyElements without a good reason for it. I'd recommend passing it in early, e.g. in constructors.

[asmecher]

@ctgraham, this is going to need a dust-off and port to OMP. Are you willing & able to tackle either/both of those? I can commit to getting it merged ASAP. (I don't think it interferes with the other big merges in a major way.)

[ctgraham]

@asmecher, rebasing shouldn't be a problem; porting to OMP will take some extra time. I presume you prefer I wait to rebase until the OMP work is done?

[asmecher]

I'd like to merge for OJS and OMP at the same time, in order to avoid breaking the master branch of either app. I don't really have a preference for the order.

[asmecher]

@ctgraham, I'm sorry if I lost sight of this! Was it ready for review/merge when you last worked with it?

[NateWr]

@ctgraham is this work still in progress or abandoned? I'm happy to keep this open if you think it's still going to see work. On the other hand, we will eventually move away from grids and similar components, so the cost v benefit may be waning.

[ctgraham]

This was mired in some strange Travis testing behavior and is now to stale for me to readily pick it back up. It should be considered abandoned.