Skip to content

MNT Standardise modules#89

Draft
emteknetnz wants to merge 1 commit intosilverstripe:masterfrom
creative-commoners:pulls/master/standardise-modules
Draft

MNT Standardise modules#89
emteknetnz wants to merge 1 commit intosilverstripe:masterfrom
creative-commoners:pulls/master/standardise-modules

Conversation

@emteknetnz
Copy link
Copy Markdown
Member

@emteknetnz emteknetnz commented Jul 31, 2022

@GuySartorelli
Copy link
Copy Markdown
Member

At least one CI job has failed (in CI org)

@emteknetnz
Copy link
Copy Markdown
Member Author

Note - closed previous PR because this required a non-standard CI file - should probably split to own card to add CI here

@emteknetnz emteknetnz force-pushed the pulls/master/standardise-modules branch from b0b1d2d to 165be39 Compare August 1, 2022 06:47
@emteknetnz emteknetnz marked this pull request as draft August 1, 2022 06:48
@emteknetnz
Copy link
Copy Markdown
Member Author

emteknetnz commented Aug 1, 2022

Module is ancient, used tabs not spaces so every line was changed by phpcbf. Also used phpunit 3. Was built for CMS 3, not idea if this still functions correctly in CMS 4, particularly DatabaseConnector.php which has use DB - I don't know if this magically auto-maps to SilverStripe\ORM\DB. There is support for upddated static function names e.g. Both older getConn and newer get_conn should both work.

There's a very non-standard thing bin/build-phar which created sspak.phar and the intention was to regularly rebuild the sspak.phar. This I don't think this actually functions as the integration in travis looks broken, even though the build there is green.

I couldn't get SmokeTest.php to work as my local php.ini wouldn't let me create a phar when trying to run bin/build-phar

I've done some work around getting the PSR4 loader working, adding phpcs, getting phpunit9 working, adding ci.yml. However it feels very messy and incomplete and I've done zero manual testing. I'm not attached to anything here so I'm happy to throw this PR out and start again and just copy some bits from this PR

We'll need to spend some working out what we want to do here

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants