Replace Sami by Doctum

williamdes commented 4 years ago

Re-title needed for https://github.com/silverstripe/api.silverstripe.org/issues/47 Closes: https://github.com/silverstripe/api.silverstripe.org/issues/51 Fixes: https://github.com/silverstripe/silverstripe-framework/issues/9422 Fixes: #91 Ping on https://github.com/silverstripe/api.silverstripe.org/issues/56 after merge to say: "go open issues upstream"

Dev notes: the diff you can see contains new symlinks intead of copied files: after all why copy pasting vendor files if we can symlink them ?

Work done:

I copied https://github.com/silverstripe/api.silverstripe.org/pull/89/files#diff-d8c06bafd0eb48321018056c54274ffd from @unclecheese

Synced vendor files when needed and restored small changes between Doctum and this repo (the custom navbar):

rm -rf conf/themes/silverstripe/*
cp -rp vendor/code-lts/doctum/src/Resources/themes/default/* conf/themes/silverstripe/

Replaced Sami by Doctum
Improved the Update script (4d8a7d8f107431268f74dfa33857553748c2367f)
Made TravisCI use PHP 7.4
:hole: Digged into https://github.com/search?q=org%3Asilverstripe+sami&state=open&type=Issues to find things to link or fix
Upgraded phpcs and phpunit

williamdes commented 4 years ago

Time to sleep, I hope the build will succeed ! :fr: Anyway, I updated the PR description to make it quite complete.

dhensby commented 4 years ago

It's green 🎉 💚

williamdes commented 4 years ago

Hi, Anyone would like to hit the merge button ;p ?

Have a nice day

sminnee commented 4 years ago

The code looked good but I wanted to check it in a test env before merging myself, which I expect others are also thinking. I hadn't got a chance to get a dev env going for this just yet, and I hadn't familiarised myself with the coverage of the test suite, so less keen to just press go without smoke-testing locally ;-)

Thanks, Sam

chillu commented 3 years ago

Looks pretty close, great job! (left old, right new)

Tried this out locally with the most basic Lando config:

name: api
recipe: lamp
config:
  webroot: '.'

proxy:
  appserver:
    - api.lndo.site

Some observations:

The menu include doesn't work (not quite sure why, might be CORS or something)
The custom "Config options" (@config) no longer works (example). Not a blocker, but that's quite useful.
The URL structure looks to be the same, so the search.php script should still function
I couldn't validate the search/lookup endpoint (current working example), but that's likely because of my local Lando setup - doesn't appear that you've changed anything there.

I've just deployed this to https://silverstripe.cloud/naut/project/ssapi2/environment/uat/overview/, which is access protected to staff (sorry!). Can't login to the box myself to trigger the task, will see what happens at the daily cron (3am).

williamdes commented 3 years ago

Hi @chillu Thank you for the review, could you try something: clean all files and regenerate the Sami version and then we can compare what is broken ?

Because from my testing it seems I had another version of the include toolbar and it felt broken in someway

After that I will have to have a look into https://github.com/silverstripe/silverstripe-admin/blob/1a7611bda9de51c184702cc08cb9798993fa97fb/code/LeftAndMain.php#L245 that does not render

chillu commented 3 years ago

Cron didn't run on our UAT last night, I've just deployed another .platform.yml tweak and will check again on Monday. So cron not running on UAT means I can't validate that it's working on the actual infrastructure, but I'm pretty confident based on what I see in my local Lando setup (it's just HTML files after all).

On the menu include, it's indeed CORS:

Refused to display 'https://www.silverstripe.org/toolbar/profile' in a frame because it set 'X-Frame-Options' to 'SAMEORIGIN'.

I'm not sure why that breaks rendering of the included HTML though. Don't think it's related to any leftover state from previously generated files.

https://github.com/silverstripe/api.silverstripe.org/blob/83fd43faa3adbf1f6e73a04ade3e225365255990/conf/themes/silverstripe/layout/layout.twig#L35

This file is generated through https://github.com/silverstripe/silverstripe-globaltoolbar - it's all a bit arcane, sorry. Any ideas?

williamdes commented 3 years ago

Cron didn't run on our UAT last night, I've just deployed another .platform.yml tweak and will check again on Monday. So cron not running on UAT means I can't validate that it's working on the actual infrastructure, but I'm pretty confident based on what I see in my local Lando setup (it's just HTML files after all).

On the menu include, it's indeed CORS:

Refused to display 'https://www.silverstripe.org/toolbar/profile' in a frame because it set 'X-Frame-Options' to 'SAMEORIGIN'.

I'm not sure why that breaks rendering of the included HTML though. Don't think it's related to any leftover state from previously generated files.

https://github.com/silverstripe/api.silverstripe.org/blob/83fd43faa3adbf1f6e73a04ade3e225365255990/conf/themes/silverstripe/layout/layout.twig#L35

This file is generated through https://github.com/silverstripe/silverstripe-globaltoolbar - it's all a bit arcane, sorry. Any ideas?

Can I make a pull-request to change the global toolbar or is it used somewhere else?

Is it possible that you infrastructure has access to one version of the toolbar and our public tests have access to another one?

Anyway I will need to sort our this toolbar so it works for good. I already spent some hours making it work ^^

chillu commented 3 years ago

Can I make a pull-request to change the global toolbar or is it used somewhere else?

Actually, I think this might be the end of that toolbar - we've already decided to remove it from https://docs.silverstripe.org/ when that was rebuilt on Gatsby. @unclecheese Did you have any discussions with PUX on that toolbar? @williamdes I'd be OK if you just replaced it with a header that looks similar to the docs site. Whatever is less work :)

The cron fired (see internal log entry), but it didn't seem to write any new files. No errors either. Will have to wait to get full infrastructure access to diagnose that.

williamdes commented 3 years ago

Update: I will work on this ASAP but I am missing some time for such tasks.

chillu commented 3 years ago

I've figured out why the cron isn't running: You're using symlinks in the git repo, but our tgz archive based deployment mechanism doesn't seem to support those (most likely for security reasons). Could we just copy the files?

sudo -u www-data '/var/www/mysite/www/makedoc.sh'
...
In FilesystemLoader.php line 250:

  [Twig\Error\LoaderError]                                                                                            
  Unable to find template "index.twig" (looked into: /var/www/mysite/releases/c8f34569ee97f5a692f34a8e60fe542d33060e  
  fd/conf/themes/silverstripe, /var/www/mysite/releases/c8f34569ee97f5a692f34a8e60fe542d33060efd/conf/themes).

ls -ahl conf/themes/silverstripe/
total 40
drwxr-xr-x@ 19 ingo  staff   608B 16 Oct 13:54 .
drwxr-xr-x@  3 ingo  staff    96B  8 Jul  2019 ..
lrwxr-xr-x   1 ingo  staff    71B 16 Oct 13:54 class.twig -> ../../../vendor/code-lts/doctum/src/Resources/themes/default/class.twig

williamdes commented 3 years ago

I've figured out why the cron isn't running: You're using symlinks in the git repo, but our tgz archive based deployment mechanism doesn't seem to support those (most likely for security reasons). Could we just copy the files?

sudo -u www-data '/var/www/mysite/www/makedoc.sh'
...
In FilesystemLoader.php line 250:

  [Twig\Error\LoaderError]                                                                                            
  Unable to find template "index.twig" (looked into: /var/www/mysite/releases/c8f34569ee97f5a692f34a8e60fe542d33060e  
  fd/conf/themes/silverstripe, /var/www/mysite/releases/c8f34569ee97f5a692f34a8e60fe542d33060efd/conf/themes).

ls -ahl conf/themes/silverstripe/
total 40
drwxr-xr-x@ 19 ingo  staff   608B 16 Oct 13:54 .
drwxr-xr-x@  3 ingo  staff    96B  8 Jul  2019 ..
lrwxr-xr-x   1 ingo  staff    71B 16 Oct 13:54 class.twig -> ../../../vendor/code-lts/doctum/src/Resources/themes/default/class.twig

Ah, good catch ! Copying all files disturbs me because it could cause you to have bugs on new versions if I change the files

I opened https://github.com/code-lts/doctum/issues/14 to try fix this issue

chillu commented 3 years ago

@williamdes Might be a necessary evil to copy the files now? You could write a script and add a reminder to the README? At the moment we could technically also run it as part of make build, but it's a bit of an anti pattern to modify the codebase on the production filesystem, so ideally we'd have those changes tracked through git.

williamdes commented 3 years ago

@williamdes Might be a necessary evil to copy the files now? You could write a script and add a reminder to the README? At the moment we could technically also run it as part of make build, but it's a bit of an anti pattern to modify the codebase on the production filesystem, so ideally we'd have those changes tracked through git.

Ha, I found out themes can have parents. https://github.com/code-lts/doctum#creating-a-theme So I used the default parent and that works fine, running Doctum throws errors on my end about NodeVisitor.php. I will have to investigate that, but maybe Doctum will work on your infrastructure now :pray:

chillu commented 3 years ago

Sweet, yeah that worked thanks William! Now the last thing left is to remove the broken header toolbar, right? I'd be OK to just use the same (simple) header that we have on docs.silverstripe.org

williamdes commented 3 years ago

I am working on fixes upstream and new errors because you have some bad hints in the codebase. (they are making some PHP exceptions occur) Here is some examples:

ERROR: [Hint] Hint is wrong ("int.")  for getPageLength at line 63 on "PaginatedList::getPageLength" in /mnt/Dev/@code-lts/@doctum-fork/api.silverstripe.org/src/Data/../../data/packages/silverstripe/framework/core/PaginatedList.php:63
ERROR: [Hint] Hint is wrong ("")  for run at line 60 on "SilverStripe\Dev\BuildTask::run" in /mnt/Dev/@code-lts/@doctum-fork/api.silverstripe.org/src/Data/../../data/packages/silverstripe/framework/src/Dev/BuildTask.php:60
ERROR: [Hint] Hint is wrong ("array;")  for hidden_permissions at line 107 on "SilverStripe\Security\Permission::$hidden_permissions" in /mnt/Dev/@code-lts/@doctum-fork/api.silverstripe.org/src/Data/../../data/packages/silverstripe/framework/src/Security/Permission.php:107
ERROR: [Hint] Hint is wrong ("$this;")  for setSuccessUrl at line 364 on "SilverStripe\Security\Confirmation\Storage::setSuccessUrl" in /mnt/Dev/@code-lts/@doctum-fork/api.silverstripe.org/src/Data/../../data/packages/silverstripe/framework/src/Security/Confirmation/Storage.php:364
ERROR: [Hint] Hint is wrong ("$this;")  for setFailureUrl at line 387 on "SilverStripe\Security\Confirmation\Storage::setFailureUrl" in /mnt/Dev/@code-lts/@doctum-fork/api.silverstripe.org/src/Data/../../data/packages/silverstripe/framework/src/Security/Confirmation/Storage.php:387

chillu commented 3 years ago

@williamdes Are these errors blocking this PR? My assumption was that those aren't breaking the generation task. I mean, you're welcome to fix those in the underlying repos, but unless that's a blocker here I'd say its lower priority.

Did you get anywhere with the header toolbar removal?

williamdes commented 3 years ago

@williamdes Are these errors blocking this PR? My assumption was that those aren't breaking the generation task. I mean, you're welcome to fix those in the underlying repos, but unless that's a blocker here I'd say its lower priority.

Did you get anywhere with the header toolbar removal?

On my workstation this is giving a php fatal error, so this was blocking me to dev the toolbar. I can remove the toolbar and add it back in another PR if you prefer?, else It will take me some more time to finish the PR but I am working on it currently.

chillu commented 3 years ago

On my workstation this is giving a php fatal error, so this was blocking me to dev the toolbar.

Seems weird that a docs generator would give you fatal errors when it discovers "hint is wrong", but I'm not across the details, sorry. When I ran Doctum locally a while ago on your PR I saw a lot of errors, but none which stopped generating the docs.

I can remove the toolbar and add it back in another PR if you prefer

Yeah I think that's what we landed on, just try to replicate the same simple styling from docs.silverstripe.org - no need to fix the toolbar itself.

williamdes commented 3 years ago

Yeah I think that's what we landed on, just try to replicate the same simple styling from docs.silverstripe.org - no need to fix the toolbar itself.

Okay, I will work on this task

Seems weird that a docs generator would give you fatal errors when it discovers "hint is wrong", but I'm not across the details, sorry. When I ran Doctum locally a while ago on your PR I saw a lot of errors, but none which stopped generating the docs.

Well the error is what I added because the hint is wrong and would not parse so it did throw a PHP error. I had to fix that issue.

Working on a bunch of internal Doctum code changes, but this should land in some days.

williamdes commented 3 years ago

Hi @chillu I did drop the toolbar in 51c9d3d3e71b892e7a924bad1b2eb6d1ab9fbf64 and only did keep the doctum.js file you modified

Do I need to do more changes ?

chillu commented 3 years ago

Hey William, sorry for the delayed response, I was on leave. Thanks again for persisting on this, we're very close! I had to fix the doctum.php invocation, and not quite sure why/how it worked before - see https://github.com/silverstripe/api.silverstripe.org/commit/8b05fbf325d67556593b68215b70175e9d8277c2. Was the conf/doctum.php a default before? Maybe that changed with a recent release? Anyway, seems to be working now.

Otherwise this is working great. Silverstripe staff can review it at https://ssapi2-uat.sites.silverstripe.com/4/SilverStripe.html (IP allowlisted)

@silverstripe/ux-contributors The header was a bit of a victim of getting this over the line. It's a pretty low key surface, so my view is that full branding isn't essential. Do you think it's worth adding a header improvement to the CMS Squad backlog? I'm not keen to get the full toolbar back on there. We've already removed it from docs.silverstripe.org, it breaks on responsive mode, and API was the last user. It's just a bit of unnecessary complexity that we don't have the resources to maintain.

Before:

After:

The last outstanding bit I can see is to reintroduce the "Config Options" section (@config type hint), see example. As I said, that's not a blocker for merge though, so I'm creating as a separate task: https://github.com/silverstripe/api.silverstripe.org/issues/91

williamdes commented 3 years ago

I had to fix the doctum.php invocation, and not quite sure why/how it worked before - see 8b05fbf. Was the conf/doctum.php a default before? Maybe that changed with a recent release? Anyway, seems to be working now.

This is quite interesting, I would need to investigate if I did not introduce a BC break recently. But your fix is correct, I did not find by what magic this was working before :O I added your fix on my branch

Thanks again for persisting on this, we're very close!

Yay ! I really want this to be merged ;)

Config options is back in 6537d96196b33d9e3709241690e7db73a839ecaf Fixes: #91

chillu commented 3 years ago

Alright, this looks great! I've merged in that last fix, which closed the PR. Redeploying to UAT now, and if that goes well it'll go onto Prod.

williamdes commented 3 years ago

Let me know If I still have something todo here ;)

chillu commented 3 years ago

Live! https://api.silverstripe.org/4/index.html?cachebuster

@williamdes Thanks again for your hard work on this! ⭐️🚀 If you ever get bored, you could look at the parse errors and fix them in our module PHPDocs. Stuff like this:

ERROR: The "field" @param tag variable name is wrong (should be "method") on "ViewableData::deprecatedCachedCall"

But absolutely no expectations, you've got your own OSS project to maintain :D

sminnee commented 3 years ago

Woohoo, awesome! :-)

williamdes commented 3 years ago

If you ever get bored, you could look at the parse errors and fix them in our module PHPDocs.

Sure, I did procrastinate on some bug fixes and built https://github.com/marketplace/actions/action-doctum :smile: It is possible that I open a pull-request on the framework repo to add it and remove phpdoc errors ?

sminnee commented 3 years ago

Is the idea that we replace the cronjobs of this site with a GitHub action on the source repo? Or would you apply the GitHub action to this repo and it would pull the other repos in?

williamdes commented 3 years ago

Is the idea that we replace the cronjobs of this site with a GitHub action on the source repo? Or would you apply the GitHub action to this repo and it would pull the other repos in?

Well you can do what you want basically But this is a good idea, you can migrate this repository to github actions And also what was my initial idea: add a lint step on the other repos to be sure the phpdoc are always well written

sminnee commented 3 years ago

Ah right yes IMO a lint step on the framework build would be useful, thanks! Did you want to write an issue on the framework repo or should I?

williamdes commented 3 years ago

Ah right yes IMO a lint step on the framework build would be useful, thanks! Did you want to write an issue on the framework repo or should I?

Writing a pull-request would be quicker 😂

But you can open an issue to fix all the phpdoc mistakes and add the lint step :)

chillu commented 3 years ago

Is the idea that we replace the cronjobs of this site with a GitHub action on the source repo? Or would you apply the GitHub action to this repo and it would pull the other repos in?

I'm not sure that's a good idea. How would those generated HTML files get from the Github actions output onto a Silverstripe Cloud environment? I certainly don't want to commit them back to the repo. Our hosting environment allows "package deployments" via uploading archives for the whole site, but not for a single folder within the webroot (one per version and repo). Overall, this just sounds like a lot of busywork for no gain.

I've raised this mainly to fix the actual PHPDoc errors in the repos, rather than change the mechanism by which files are generated. Running a Doctum sanity check via Github Actions to ensure we don't regress once we fixed existing PHPDoc errors would be a nice addition to that of course.

williamdes commented 3 years ago

I'm not sure that's a good idea. How would those generated HTML files get from the Github actions output onto a Silverstripe Cloud environment? I certainly don't want to commit them back to the repo. Our hosting environment allows "package deployments" via uploading archives for the whole site, but not for a single folder within the webroot (one per version and repo). Overall, this just sounds like a lot of busywork for no gain.

I understand your point of view, the action was not designed to replace your deployment process anyway :) But anyone here can use it if it fits the needs to lint, parse, update, ...

sminnee commented 3 years ago

Feel free to do a PR instead of an issue but basically it was a place to have the "is this a good idea?" discussion.

I think "framework code is designed to successfully produce API docs" is a reasonable lint constraint to put on the framework package, but it should just be a linter rather than pre-generating the docs there.

I'm not sure I fully understand what you have in mind though so maybe a PR will clarify that.

silverstripe / api.silverstripe.org

Replace Sami by Doctum #90

Replace Sami by Doctum