-
Notifications
You must be signed in to change notification settings - Fork 448
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
[OMP] Fatal error when creating a series menu item without content #10511
Comments
…rt a menu item with the series type. Signed-off-by: yves <[email protected]>
Hi @jonasraoni ! 😃 |
I've checked your PR and it makes sense, I'll update and approve it. Nevertheless, I'll leave this issue open, it might be possible to create a menu without series yet (e.g. if you create the menu before creating a series) and there's also the people who already have a bad menu on the database. |
@jonasraoni I tested how it works when no series are present, and there is no option to create a series menu if no series exists, so that case already seems to be covered. |
Great, so there's just one thing to fix, the existing bad records. |
…map with keys. Co-authored-by: Jonas Raoni Soares da Silva <[email protected]>
pkp/pkp-lib#10511 menu item of type series without content even when they exist in the OMP 3.4
Thanks @YvesLepidus! Your PR is now merged to 3.4, and I can cherry-pick this to main for the 3.5 release and work on the issue with bad records. |
@kaitlinnewson I was going to handle this one, but as you've self-assigned, please take a look at the Basically, setup a bad menu, then check this line: If you can't reproduce, great 😂 |
@jonasraoni I think this is happening when Smarty is trying to convert the Collection objects into strings but failing. With the bad code, this error is logged when creating the bad series menu item:
...and this is a snippet of what gets rendered in the HTML with the bad code: <label for="seriesSelect">Select Series</label>
<div>
<select id="relatedSeriesId" name="relatedSeriesId" required="" aria-required="true" aria-invalid="false">
<option value="�*�escapeWhenCastingToString"></option>
</select> ... and as a result of this, Is anything needed beyond removing the bad entries? |
Okay, just wanted to make sure there's nothing strange under the hood 🥲 Well, I see two ways: Going further, what happens when a linked series is removed? I'm almost sure it's going to leave orphaned entries behind (which should probably be fixed as well), and I guess the same might happen with other types of menus that are linked against database entities. |
…rt a menu item with the series type. Signed-off-by: yves <[email protected]>
…map with keys. Co-authored-by: Jonas Raoni Soares da Silva <[email protected]>
I think it's better to remove them, because if they are assigned to a menu they produce a fatal error and blank white screen on the page where the menu is assigned (so they aren't usable at all). I can test the other scenario you mentioned, and file another issue for that if I find issues with orphaned entries when a series is deleted. |
(The |
Just to confirm, I deleted a series and the menu was kept, I guess other "entity links" might have the same issue (#10633). And I think it's worth to fix the origin of the issue (the |
@kaitlinnewson and @jonasraoni, this appears to have been merged/fixed in 3.4.0 but not |
@asmecher yes, this is merged for |
pkp/pkp-lib#10511 fix and cleanup invalid series navigation menu items
Describe the bug
After creating a series menu item without linking it to any item (see image below), a fatal error happens.
The strange thing that needs to be analyzed is that a call to
$navigationMenuItem->getPath()
outputs Laravel code�*�escapeWhenCastingToString
.A workaround must be left for bad cases, and the field should be probably turned into required. If the ID is a physical field, it can be setup as not null, but a further script should clear invalid data on the upgrade.
It's also needed to check if the bug applies to OMP 3.3
To Reproduce
What application are you using?
OMP 3.4
Pull requests
The text was updated successfully, but these errors were encountered: