Skip to content
This repository has been archived by the owner on Mar 16, 2023. It is now read-only.

Upgrade to SilverStripe 4 #150

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

UndefinedOffset
Copy link
Contributor

Upgraded to support SilverStripe 4.3+. Per @robbieaverill's recommendation in #133 I've removed the search functionality and related code from the module. I've also changed how the routing works to be registered with DevelopmentAdmin since I couldn't get the old routing to work at all without doing that. Perhaps it was something with how I set the after, not really sure either way having it registered with DevelopmentAdmin solves the routing.

As well exposing images for example from the documentation since the core now uses the public folder requires some more configuration on a per-case basis, meaning you must mark the folder path to be exposed like normal.

@UndefinedOffset
Copy link
Contributor Author

I'm not really sure why Scrutinizer is not happy with composer, Travis seems to have installed properly. Though I'm working on the test failures ;)

@UndefinedOffset
Copy link
Contributor Author

So the tests now pass locally because the module is not the root of the install but in the case of Travis it's the root which causes the DocumentationParserTest to fail in a couple spots. I'm torn as to whether adjust the tests or Travis (somehow).

@robbieaverill
Copy link
Contributor

I had a quick look at the test failures, and they're not really great assertions anyway to be honest. I think we should get rid of the DOCSVIEWER_DIR constant and use module references e.g. silverstripe/docsviewer instead wherever possible. In the case of those assertions, you might also be able to break up the expected string into blocks and exclude the variable part of it (the DOCSVIEWER_DIR value).

@UndefinedOffset
Copy link
Contributor Author

UndefinedOffset commented Jan 15, 2019

I agree about removing the DOCSVIEWER_DIR (same with the DOCSVIEWER_PATH) it's only used in the tests anyways but I will work towards removing it. As for solving the path failure issue in Travis, I think it's important that we do check the path re-mapping but it falls apart in Travis where the module is the root.

We could use regex to find the path with or without the vendor/silverstripe/docsviewer/ bit which would make Travis pass but also catch it when the module is properly installed. Not a great solution but it would work, ultimately I think the best route would be somehow having it properly installed into the vendor folder when Travis starts.

@UndefinedOffset
Copy link
Contributor Author

In short the url remapping would either match Travis'es resources/tests/ or resources/vendor/silverstripe/docsviewer/tests/. So technically the remapping is correct in either scenario, if you needed to expose a folder in the root of the site then the path would be right.

@robbieaverill
Copy link
Contributor

Great that you've got Travis passing. There's a conflict you'll need to resolve now as well. I'll just go and make sure that the master branch is ready for SilverStripe 4.x compat, then if you could rebase onto it that'd be awesome! Thanks again for the PR.

@robbieaverill
Copy link
Contributor

BTW you may need to update the Scrutinizer configuration file. Here's a working example for the new Scrutinizer build engine (you may need to adjust src/code folder names):

checks:
    php: true

build:
    nodes:
        analysis:
            tests:
                override: [php-scrutinizer-run]

filter:
    paths: ["src/*", "tests/*"]

Fixed pathing for the css and js requirements

Updated composer type to a vendor module

Updated the requirement for mnapoli/front-yaml to be compatible with SS4

Removed unsupported language file

Updated translations
Updated namespaces in language files
Fixed issue when under DevelopmentAdmin where the url parameters would not line up properly

Replaced get_class() to determine type with the new getType method

Relocated templates to match controller namespace
through DevelopmentAdmin

Fixed include paths in the DocumentationViewer_DocumentationPage
template

Fixed undefined variable notice
Changed branch alias to 3.x-dev
Adjusted composer requirements
Updated tests to account for the removal of DOCSVIEWER_PATH and DOCSVIEWER_DIR

Fixed test failure in DocumentationPageTest where the path on Travis would not match the normal install path
@UndefinedOffset
Copy link
Contributor Author

Re-based, though Scrutinizer did not run at all based on this force push now? It was failing seemingly due to a composer issue.

@robbieaverill
Copy link
Contributor

robbieaverill commented Apr 29, 2019

@UndefinedOffset yeah you'll need to update the Scrutinizer configuration to match the example I posted above. The config that's in this module already uses the old Scrutinizer build system which has an outdated version of Composer that isn't compatible with the recipe or vendor plugins that SilverStripe 4 uses. I've reopened the PR to see if it will re-trigger Scrutinizer, but doesn't look like it has.

@UndefinedOffset
Copy link
Contributor Author

Well the updated config seems to have triggered Scrutinizer :) it looks like Travis failed :( because it lost connection to the DB during tests? Which is a new one lol...

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

Successfully merging this pull request may close these issues.

2 participants