Skip to content

Conversation

@gko
Copy link
Member

@gko gko commented Dec 27, 2019

So that you save a particular part of Nextcloud (i.e. notes)

So that you save a particular part of Nextcloud (i.e. notes)

Signed-off-by: Konstantin Gorodinskii <[email protected]>
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Thanks for your pr 👍

Would you mind to add some insights about this change? A manifest.json per app is necessary to ...

@gko
Copy link
Member Author

gko commented Dec 28, 2019

@kesselb Thank you for the quick response. I wrote the reasoning in the second PR

Basically I want to be able to access a part of Nextcloud directly via «Add to Home screen» feature of a mobile browser.

Right now manifest is always rewritten by default app theming. It checks whether there is an existing file apps/<app>/img/manifest.json. Otherwise it generates its own manifest in the ThemingDefaults.php file:

if ($image === 'manifest.json') {
	try {
		$appPath = $this->appManager->getAppPath($app);
		if (file_exists($appPath . '/img/manifest.json')) {
			return false;
		}
	} catch (AppPathNotFoundException $e) {}
	$route = $this->urlGenerator->linkToRoute('theming.Theming.getManifest');
}

The problem here is that half of the feature was already there (if the manifest exists it stays in the meta tag) but it's then blocked (redirected) by .htaccess file.

That's why I added those two rules.

The second part is that there is never a hash added to manifest file if it exists. I fixed it in the second commit.

@gary-kim gary-kim assigned AasthaGupta and unassigned AasthaGupta Dec 28, 2019
@gary-kim
Copy link
Member

Wrong button :)

@gary-kim gary-kim added 3. to review Waiting for reviews enhancement labels Dec 28, 2019
Signed-off-by: Konstantin Gorodinskii <[email protected]>
@kesselb kesselb added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 28, 2019
@kesselb
Copy link
Contributor

kesselb commented Dec 28, 2019

Ah. Did some testing. This approach will only work for instances with mod_rewrite enabled. It does not work with nginx. RewriteRule will tell the webserver to not forward the request for /apps/notes/img/manifest.json to nextcloud and serve it statically. That's fine but we probably need a new rule for nginx. Otherwise nextcloud must be able to serve these files (but that includes booting up everything) and serve the file. That's already there for many static assets (e.g. a generic route to serve js or css from any app).

Let me ping @rullzer and @juliushaertl to ask them how to do it. The use case here is to ship a custom manifest.json for a app. URL generation seems to work already. Is this something that apps should implement theirself? Or should we tell the webservers to just serve any of these manifest.json files.

    location ~ manifest\.json$ {
        try_files $uri =404;
    }

That worked for me (with nginx) and will not break the theming manifest (because the url is something like /apps/theming/manifest?v=0). But I miss the big picture and don't know how this should integrate with theming ;)

This reverts commit d1a8af5.

Signed-off-by: Konstantin Gorodinskii <[email protected]>
@gko
Copy link
Member Author

gko commented Dec 29, 2019

@kesselb thank you for testing on nginx. I created a sandbox to test and develop nextcloud apps. But it uses the image with apache by default.

Also would it be possible to backport it to 16.x and 17.x once we manage to make it work everywhere?

$content .= "\n RewriteCond %{REQUEST_FILENAME} !core/img/favicon.ico$";
$content .= "\n RewriteCond %{REQUEST_FILENAME} !core/img/manifest.json$";
foreach (\OC::$APPSROOTS as ['url' => $url]) {
$content .= "\n RewriteCond %{REQUEST_FILENAME} !" . trim($url, '/') . "/[\w-_]+/img/manifest.json$";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could replace the foreach with a more generic RewriteCond. If someone is adding a new app_path he will probably not regenerate the htaccess. Such issues are hard to spot even if we document it somewhere. I'm not sure yet. Thats a obscure edge case of course 🤔

@rullzer
Copy link
Member

rullzer commented Jan 8, 2020

If we do this we should make it a generic route that is handled in some controller and then depending on what app you pass to it. Doing .htaccess magic is to error prone in my opinion. But lets revisit this when we start 19 development.

@kesselb
Copy link
Contributor

kesselb commented Jan 10, 2020

If we do this we should make it a generic route that is handled in some controller and then depending on what app you pass to it.

And bootstrapping the whole nextcloud stack for serving a static file. His approach is to exclude manifest.json and let the webserver handle it. I'm sure we can replace foreach appsroots with a generic rule for manifest.json.

@skjnldsv
Copy link
Member

skjnldsv commented Aug 2, 2020

Bump?

@szaimen
Copy link
Contributor

szaimen commented Jun 23, 2021

@gko any update here? :)

@szaimen szaimen linked an issue Jun 23, 2021 that may be closed by this pull request
@szaimen
Copy link
Contributor

szaimen commented Aug 28, 2021

Superseded by #28459

@szaimen szaimen closed this Aug 28, 2021
@szaimen szaimen removed this from the Nextcloud 23 milestone Aug 28, 2021
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.

Enable using the NextCloud apps as apps via Google Chrome shortcuts

7 participants