Skip to content

Conversation

@Pytal
Copy link
Member

@Pytal Pytal commented Jan 31, 2023

Summary

Some apps register navigation entries with empty routes e.g.

This allows these entries to pass xmllint used in https://github.com/nextcloud/.github/blob/ff4a6f1eacb53e4d3b5721e57743eb64242178a7/workflow-templates/lint-info-xml.yml

Checklist

@Pytal Pytal added 3. to review Waiting for reviews feature: settings labels Jan 31, 2023
@Pytal Pytal added this to the Nextcloud 26 milestone Jan 31, 2023
@Pytal Pytal self-assigned this Jan 31, 2023
@Pytal
Copy link
Member Author

Pytal commented Jan 31, 2023

/backport to stable25

@nickvergessen
Copy link
Member

nickvergessen commented Jan 31, 2023

  • patch also in appstore, or is it intentionally that only the shipped app file is modified

@nickvergessen
Copy link
Member

  • patch also in appstore

</xs:simpleType>

<xs:simpleType name="route">
<xs:restriction base="non-empty-string">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead allowing empty, just allow no occurrences of the route tag?🤔
Sounds easier and more straight forward, but might require additional PHP fallbacks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was exactly the initial idea, but because this is needed for #36232 for NC25 accessibility, the minimally invasive solution here prevents the need to backport some potentially breaking changes to i.e. https://github.com/nextcloud/server/blob/ca3f53ab88b06c8ceea5d7991ff40641a9820728/lib/private/NavigationManager.php and https://github.com/nextcloud/server/blob/ca3f53ab88b06c8ceea5d7991ff40641a9820728/lib/private/App/InfoParser.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the necessary PHP patch:

diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php
index b78d9fa1ed8..f24a2f0ac25 100644
--- a/lib/private/NavigationManager.php
+++ b/lib/private/NavigationManager.php
@@ -315,7 +315,8 @@ class NavigationManager implements INavigationManager {
 				$id = $nav['id'] ?? $app . ($key === 0 ? '' : $key);
 				$order = isset($nav['order']) ? $nav['order'] : 100;
 				$type = isset($nav['type']) ? $nav['type'] : 'link';
-				$route = $nav['route'] !== '' ? $this->urlGenerator->linkToRoute($nav['route']) : '';
+				$route = $nav['route'] ?? '';
+				$route = $route !== '' ? $this->urlGenerator->linkToRoute($route) : '';
 				$icon = isset($nav['icon']) ? $nav['icon'] : 'app.svg';
 				foreach ([$icon, "$app.svg"] as $i) {
 					try {

@nickvergessen
Copy link
Member

nickvergessen commented Jan 31, 2023

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

- For xmllint

Signed-off-by: Christopher Ng <[email protected]>
@Pytal Pytal force-pushed the enh/allow-empty-route-xml branch from 2703dd2 to c8c010b Compare February 1, 2023 02:15
@blizzz blizzz mentioned this pull request Feb 1, 2023
@Pytal
Copy link
Member Author

Pytal commented Feb 3, 2023

Superseded by #36508

@Pytal Pytal closed this Feb 3, 2023
@Pytal Pytal deleted the enh/allow-empty-route-xml branch February 24, 2023 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants