Skip to content
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

Pico CMS for Nextcloud v1.0.0 (WIP) #77

Merged
merged 238 commits into from
Nov 25, 2019
Merged

Conversation

PhrozenByte
Copy link
Collaborator

@PhrozenByte PhrozenByte commented Jul 19, 2019

This is, without any question, a lot... 😄 This is a combination of a total refactoring and a rewrite from scratch - but IMHO we're very close to Pico CMS for Nextcloud version 1.0.0 🎊 🎉

Testing the app is pretty straight forward:

  1. Download the tarball/ZIP or clone my fork (see PhrozenByte/nextcloud-pico, branch pico-2.0) to apps/cms_pico/ of your Nextcloud instance (requires Nextcloud 14+)
  2. Run composer install
  3. Enable the app in your Nextcloud instance
  4. Check out your personal and admin settings pages of "Pico CMS"

PRs to my fork and comments/reviews are very welcome! ❤️ My fork also allows edits from maintainers.

This PR solves all known (at least to me... I'll reference some issues in follow-up comments) issues with the current Nextcloud app, utilizes the latest release of Pico 2.1 (currently a dev build, however, I guarantee Pico 2.1 will be released before releasing this app) and I'm furthermore willing to officially support this app (Hi everyone, I'm Pico's lead dev ✌️ - i.e. Pico CMS for Nextcloud will be officially supported by Pico).

Please note that many commits are rather huge and don't really address single issues, but single aspects of the app. This is rather bad for change traceability, but IMHO contributes to the final solution, since it allowed me to ignore limitations of the existing code base. Just to mention that a thousand small commits with a tons of reverts won't be much better in regards of change traceability... So... 🤷‍♂️ However, naturally I didn't re-invent the wheel here and changed only a few concepts, not all of them 😉 Some commits do have some elaboration on my thoughts.

@daita you really did an amazing job here! Great work! ❤️ I hope you forgive me for being so boldly doing this... You invested so much time, but priorities change (or, better to say I guess, the priorities of your employer change 🙈) and I felt like this app fell a bit behind 😞 So many people asked me about the plans for the Nextcloud app and I just could only answer "I don't know". Since it's an amazing project and heavily contributes to Pico's visibility, I didn't want to let it fall behind... And here we are. Thanks for all of your great work @daita! 👍


Screenshots:

Example website

Example website

List websites

List websites

Create website

Create website

Custom themes, plugins and templates

Custom themes, plugins and templates

Webserver config

Webserver config


Some thoughts about some particular features:

  • Pico CMS for Nextcloud now supports both mod_proxy-based URLs (like https://example.com/sites/my_pico_site) and generic URLs (like https://example.com/index.php/apps/cms_pico/pico/my_pico_site). Currently the latter is shown by default (since it works out-of-the-box and webserver configuration was the most often asked question and source of problems until now) and the shorter form is just mentioned. I'll add a simple config option to enable short URLs after the admin has configured the webserver appropriately. - Done 👍

  • I'm planning to add a "Add custom plugin" feature just like for themes, allowing admins to add plugins to Pico. Plugins are tested for compatibility (Pico will throw an exception otherwise). We might even extend this to enabling plugins on a per-website basis. However, the latter is still a long way off. - Done 👍

  • Handling assets (content assets, theme assets and plugin assets) still isn't optimal (especially regarding browser caching). To solve the remaining issues I'll add a feature to Pico to support %assets_url% and %plugins_url% next to the already existing %theme_url% (+ corresponding Twig variables; actually this is already implemented in Pico 2.1). These variables can then be used by Pico CMS for Nextcloud to create version-specific URLs utilizing Nextcloud's ETag (content assets) resp. some random string that is generated when a theme/plugin is published or updated. Not sure about the details yet. - Done 👍

  • The two features listed above are currently just available for Pico plugins. I'll enable the same functionality for Pico themes, too. - Done 👍

  • I'm no big fan of unit tests (not because I don't like them, I usually just don't have time writing them...). All existing tests are still working, but I didn't write any new.

  • I don't really know how Transifex works 😕 de.js, de.json, de_DE.js, de_DE.json, en_GB.js and en_GB.json are all fully up-to-date, but how can we import them to Transifex?

  • I've absolutely no idea how this whole release process works (.drone.yml ❓)...

@PhrozenByte PhrozenByte changed the title 🚧 🎉 Pico CMS for Nextcloud v1.0.0 (WIP) Pico CMS for Nextcloud v1.0.0 (WIP) Jul 19, 2019
Don't pin a specific Pico version, always use the latest stable version. However, since Pico might maintain BC between minor versions using PicoDeprecated (which we don't wanna use), we can't fully rely on semantic versioning and must manually upgrade to Pico 2.1 (which might never exist...). Thus we use the version constraint '~2.0.0'.

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…eption

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…undException

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…eption

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…ception

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…tedException

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…dException

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…edException

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…xception

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
This is a complete code refactoring of how the Nextcloud app handles and passes requests to Pico. It solves all currently known issues with the Nextcloud app and introduces a straight-forward request pipeline.

This was written mostly from scratch, thus there's unfortunately no chance to split this up into smaller commits...

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Update file license headers in accordance to https://github.com/nextcloud/server/blob/master/contribute/HowToApplyALicense.md

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Instead of routing all HTTP requests for theme assets through Nextcloud (that's super slow...), we rather copy all themes to the new `appdata_public/` folder. The app recreates the `appdata_public/` folder on installation/upgrade using the `AppDataRepairStep` repair step. System themes (i.e. Pico's default theme) are taken into account, but are no longer copied to the appdata folder. Otherwise it would be impossible to update system themes. System templates and Pico's config files are still copied to the appdata folder using the `AppDataRepairStep` repair step. The `Pico/` folder is renamed to `appdata/` for consistency reasons. Adding a new custom theme simply copies the corresponding folder from the appdata folder to `appdata_public/` (i.e. the theme is published). When removing a custom theme, its corresponding folder in the `appdata_public/` directory is removed (i.e. the theme is depublished).

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…ustContains()

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…sertValidTemplate()

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Also see 53f0745

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Looks like that PostgreSQL doesn't like table aliases under some circumstances, even though Doctrine should take care of this. Anyway, the aliases aren't necessary since we don't use JOINS or subqueries, so we can simply remove them. Also see https://help.nextcloud.com/t/beta-needs-testing-pico-cms-for-nextcloud-v1-0-0/58718/17

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Even though Pico 2.1.0 is a stable release, we must keep { "minimum stability": "beta" } in Pico CMS for Nextcloud's composer.json. This is due to Parsedown 1.8 being in beta right now. Unfortunatly Composer can't really decide this on a per-dependency basis... However, due to { "prefer-stable": true }, Composer should still download the latest stable releases whenever possible.

Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
PhrozenByte added a commit to PhrozenByte/nextcloud-pico that referenced this pull request Nov 25, 2019
@PhrozenByte PhrozenByte merged commit b3cfdcf into nextcloud:master Nov 25, 2019
@PhrozenByte
Copy link
Collaborator Author

PhrozenByte commented Nov 25, 2019

The final release of Pico CMS for Nextcloud v1.0.0 is out now! 🎊 🎉

You can download Pico CMS for Nextcloud's release archive from here: cms_pico-1.0.0.tar.gz

Heads up! If you're currently running Pico CMS for Nextcloud v1.0.0-beta.1 you MUST upgrade to v1.0.0-beta.2 first before upgrading to v1.0.0 later - you'll break your installation otherwise! Please refer to the announcement of v1.0.0-beta.2 for more details.


Full Announcement

Please refer to the full announcement of Pico CMS for Nextcloud v1.0.0 for more details:

➡️ Pico CMS for Nextcloud v1.0.0 is out now on Nextcloud Help ⬅️

If you are a beta tester, head over to the announcement for beta testers on Nextcloud Help.

@PhrozenByte
Copy link
Collaborator Author

Just for reference: I just "imported" the development translation files (l10n/de.json, l10n/de_DE.json and l10n/en_GB.json) into Transifex using the following "script" on the command line (super dirty, but does the job):

$ php -r '$l10n = json_decode(file_get_contents("l10n/de.json"), true)["translations"]; foreach ($l10n as $msgid => $msgstr) { echo "msgid \"" . str_replace("\"", "\\\"", $msgid) . "\"\n"; echo "msgstr \"" . str_replace("\"", "\\\"", $msgstr) . "\"\n\n"; }'

One just has to download the "Download file to translate" on Transifex and replace all language strings with the output of the above command. Upload the resulting .po file using the "Upload file" on Transifex and you're ready to go.

@PhrozenByte
Copy link
Collaborator Author

PhrozenByte commented Nov 27, 2019

@MorrisJobke wrote at #77 (comment)

  • I don't really know how Transifex works confused de.js, de.json, de_DE.js, de_DE.json, en_GB.js and en_GB.json are all fully up-to-date, but how can we import them to Transifex?

We extract source strings from the app and push them to transifex. Then we fetch updated translations from transifex and convert them into our format. Thus changes to translations will be overwritten by this sync and should be made on transifex once this PR is merged and thus has the updated source strings. If you really really want to import them, then we need to write a script that converts those to a PO file and then we can do a one time transifex import, but this will forcefully overwrite all strings on transifex and should be handled with care. I would prefer to manually go over them on the transifex side (there are also people that might help you with that)

Looks like Transifex wasn't able to import all l10n strings, a few are missing (because the untranslated string are stored in the appconfig and passed to IL10N later). How can I add some strings to Trasifex manually, or is there a way/syntax how I can explicitly tell Transifex that a arbitrary string is indeed a l10n string?

Here's an example:

throw new PluginNotCompatibleException(
$this->getName(),
'Incompatible plugin: Plugin class "{class}" not found.',
[ 'class' => $className ]
);

I was thinking about some sort of comment syntax that tells Transifex that there's an l10n string, maybe something like this

				// L10N: Incompatible plugin: Plugin class "{class}" not found.
				throw new PluginNotCompatibleException( 
					$this->getName(), 
					'Incompatible plugin: Plugin class "{class}" not found.', 
					[ 'class' => $className ] 
				);

How can I achieve this?

@PhrozenByte
Copy link
Collaborator Author

@MorrisJobke

I've got another issue with Transifex: Looks like I don't have elevated rights for the cms_pico project (my username there is PhrozenByte, too). I can't change l10n strings, only draft suggestions. I think I can't review l10n strings either. How can I get those elevated rights?

@Ludovicis
Copy link

Hello PhrozenByte,

It's OK also for the French language in transifex (But no reviewed, i don't know who is the examiner...).
Thanks again for your work

@juliusknorr
Copy link
Member

@jospoortvliet probably wants to do some marketing about the new app release 😉

@jospoortvliet
Copy link
Member

jospoortvliet commented Dec 6, 2019

Yes! We should talk about this! @Ludovicis please please email me (my first name @ the nextcloud.com server) - we should blog. Pretty please because this is awesome 🚀

Edit - I meant @PhrozenByte of course... sorry for the mixup!

@jospoortvliet
Copy link
Member

@PhrozenByte 👆👆👆

@PhrozenByte
Copy link
Collaborator Author

@jospoortvliet Sorry, missed your edit. Just sent you an email. Looking forward to it, thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants