Closed ctgraham closed 2 years ago
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
Looks like GridCellProvider::render()
expects $row
as the first argument (and getId()
is called on it), but we’re now passing a request.
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.
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!
GridCategoryRowCellProvider::render()
and GridHandler::_renderCellInternally()
are culprits… shall I fix them, or will you?
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.
Let me finish tracking this further upstream and see where you stand…
Ugh… it’s a little more global than just the calls to render()
. GridCategoryCellProvider
has subclasses, many of which are explicitly passed a request.
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
Where is GridCategoryCellProvider
defined? I'm not seeing it.
It’s GridCategoryRowCellProvider
, in classes/controllers/grid
. Sorry for the typo. But I meant GridCellProvider
, anyway, that has subclasses.
I think the next step (for next week) is to identify whether GridRow
s and GridColumn
s 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
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.
Found a few lacunae to check in.
Also dependencies in other repos:
pkp/staticPages
StaticPageGridCellProvider
StaticPageGridHandler
pkp/translator
LocaleFileListbuilderGridCellProvider
LocaleFileListbuilderHandler
LocaleGridHandler
@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 GridBodyElement
s to the initialize()
method instead. Is that legitimate? Do you have any sense of how disruptive that will be?
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.
I’ve also attempted to audit all the uses of GridCellProvider
and its subclasses, and they all seem to be clean now.
@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 Handler
s) and I wouldn't want to replicate that all the way down the stack into e.g. GridBodyElement
s without a good reason for it. I'd recommend passing it in early, e.g. in constructors.
@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.)
@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?
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.
@ctgraham, I'm sorry if I lost sight of this! Was it ready for review/merge when you last worked with it?
@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.
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.
From https://github.com/pkp/pkp-lib/issues/1923#issuecomment-278471542:
This is a blocker for pkp/pkp-lib#1923.