pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
307 stars 448 forks source link

Unable to upload images to custom nav item at site level #10578

Open NateWr opened 3 weeks ago

NateWr commented 3 weeks ago

Describe the bug I want to upload an image to a custom page navigation menu item. This is possible when using a custom page at the journal level, but not at the site level.

To Reproduce Steps to reproduce the behavior:

  1. Make sure you have more than one journal.
  2. Go to Administration > Site Settings > Site Setup > Navigation.
  3. Click Add Item.
  4. Select Custom Page for Navigation Menu Type.
  5. In the Content field, click the button to add an image.
  6. See that no Upload option is available.

Follow the same steps at the journal level to see that the image upload option is available.

What application are you using? OJS stable-3_3_0 branch.

Additional information This happens because the TinyMCE plugin doesn't add the uploadUrl to the plugin settings when no context is available. However, files are stored by user, not context, so the upload will work if enabled. PR incoming.

NateWr commented 3 weeks ago

PR: https://github.com/pkp/tinymce/pull/91 (stable-3_3_0) https://github.com/pkp/tinymce/pull/92 (stable-3_4_0) SEE NOTE: https://github.com/pkp/tinymce/pull/93 (main)

Tests only: https://github.com/pkp/ojs/pull/4500 (stable-3_3_0) https://github.com/pkp/ojs/pull/4520 (stable-3_4_0)

I can't remember whether the tests capture updates to plugin submodules or not. Will the tests above work ok or do I need to do something special?

NateWr commented 3 weeks ago

There were a few test failures but they look inconsistent to me.

asmecher commented 2 weeks ago

I would support those changes but you might need to do a small bit of digging when it comes to the main branch -- the CONTEXT_SITE constant changed with https://github.com/pkp/pkp-lib/issues/8333. The stable-3_3_0 PRs look good and are ready for forward-porting.

NateWr commented 1 week ago

@asmecher I've added PRs for each of the branches. I've manually tested stable-3_3_0 and stable-3_4_0. However, I wasn't sure what the correct contextPath for the site-level API was, so I used Application::SITE_CONTEXT_PATH. I was unable to test this because editing a navigation menu item at the site is broken for me. This may be due to an out-of-sync database, but I suspect it's a side-effect of #8333.

Steps to reproduce:

  1. Make sure you have more than one journal in a site.
  2. Go to Administration > Site Settings > Site Setup > Navigation.
  3. Click Add Item
  4. See error.

Here is the error in my logs:

[14-Nov-2024 15:38:40 UTC] PHP Fatal error:  Uncaught TypeError: PKP\controllers\grid\navigationMenus\form\PKPNavigationMenuItemsForm::__construct(): Argument #1 ($contextId) must be of type int, null given, called in /lib/pkp/controllers/grid/navigationMenus/NavigationMenuItemsGridHandler.php on line 240 and defined in /lib/pkp/controllers/grid/navigationMenus/form/PKPNavigationMenuItemsForm.php:40
Stack trace:
#0 /lib/pkp/controllers/grid/navigationMenus/NavigationMenuItemsGridHandler.php(240): PKP\controllers\grid\navigationMenus\form\PKPNavigationMenuItemsForm->__construct(NULL, 0)
#1 [internal function]: PKP\controllers\grid\navigationMenus\NavigationMenuItemsGridHandler->addNavigationMenuItem(Array, Object(APP\core\Request))
#2 /lib/pkp/classes/core/PKPRouter.php(329): call_user_func(Array, Array, Object(APP\core\Request))
#3 /lib/pkp/classes/core/PKPComponentRouter.php(265): PKP\core\PKPRouter->_authorizeInitializeAndCallRequest(Array, Object(APP\core\Request), Array)
#4 /lib/pkp/classes/core/Dispatcher.php(158): PKP\core\PKPComponentRouter->route(Object(APP\core\Request))
#5 /lib/pkp/classes/core/PKPApplication.php(388): PKP\core\Dispatcher->dispatch(Object(APP\core\Request))
#6 /index.php(30): PKP\core\PKPApplication->execute()
#7 {main}
  thrown in /lib/pkp/controllers/grid/navigationMenus/form/PKPNavigationMenuItemsForm.php on line 40

I think PKPNavigationMenuItemsForm needs to accept a null context id.

asmecher commented 1 week ago

@jonasraoni, could you have a look at the above comment?

jonasraoni commented 1 week ago

I can confirm it's a side-effect, probably from a merge conflict, I'll push a fix on the other issue.

asmecher commented 6 days ago

@NateWr, I think the error you encountered should now be fixed by https://github.com/pkp/pkp-lib/pull/10619; could you try re-running?