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

SS4 update #17

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

SS4 update #17

wants to merge 9 commits into from

Conversation

micschk
Copy link

@micschk micschk commented Apr 23, 2018

No description provided.

@micschk micschk changed the title SS4 update SS4 update FIX #18 Apr 23, 2018
@micschk micschk changed the title SS4 update FIX #18 SS4 update fixes #18 Apr 23, 2018
@micschk micschk changed the title SS4 update fixes #18 SS4 update Apr 23, 2018
@micschk
Copy link
Author

micschk commented Apr 23, 2018

fixes #18

Accidentally committed temporary dev file
@nglasl
Copy link

nglasl commented Jul 3, 2018

Thanks for submitting this PR, it's greatly appreciated :)

We'll be in touch if there's any questions and/or changes required here.

@silbinarywolf
Copy link

@micschk There are a few items I'd like you to fix if you have time:

  • Remove "installer-name": "sitemap" from composer.json as it's a silverstripe-vendormodule. (So this is ignored anyway)
  • Change SitemapPageTest.yml to use namespace names. (Example below)
  • Missing seperate SitemapPageController.php file. (Example below)
  • Move Sitemap/Includes/SitemapListing.ss to Symbiote/SitemapPage/Includes/SitemapListing.ss
  • Update fixture file in SitemapPageTest.php to protected static $fixture_file = 'SitemapPageTest.yml';
  • Fix namespace of SitemapPageTest.php to be namespace Symbiote\SitemapPage\Tests;
  • Add Tests to PSR-4 of composer.json (Example below)
  • Fix failing tests. (They rely on getSitemap, which you seemingly removed?)
  • Remove _config.php, this shouldn't be needed for silverstripe-vendormodule, however you might need to create a blank _config/config.yml file for the module to be picked up by SilverStripe.
  • Fix invalid phpDoc on on Line 48 of SitemapPage.php (See exact error below)

Even if you don't have time to fix all of these, if you could commit to touching up / fixing a couple, that'd be fantastic!

Fixtures file fixed
I've noticed that the fixtures file wasn't updated to have namespacing like below:

SilverStripe\CMS\Model\SiteTree:
    home:
        Title: Home
    about:
        Title: About
    staff:
        Title: Staff
        Parent: =>SilverStripe\CMS\Model\SiteTree.about
    history:
        Title: History
        Parent: =>SilverStripe\CMS\Model\SiteTree.about
    contact:
        Title: Contact Us
Symbiote\SitemapPage\SitemapPageTest_Unviewable:
    hidden:
        Title: Hidden Page

Fix composer.json autoload

"autoload": {
	"psr-4": {
		"Symbiote\\SitemapPage\\": "src/",
		"Symbiote\\SitemapPage\\Tests\\": "tests/"
	}
},

Add missing SitemapPageController

<?php

namespace Symbiote\SitemapPage;

use PageController;

/**
 * @package silverstripe-sitemap
 */
class SitemapPageController extends PageController {
}

How I ran tests
Change phpunit.xml on a blank SilverStripe 4.1.1 project to the config below and run vendor/bin/phpunit in root project.

<phpunit bootstrap="vendor/silverstripe/cms/tests/bootstrap.php" colors="true">
    <testsuite name="Default">
        <directory>vendor/sitemap/tests/</directory>
    </testsuite>

    <php>
        <ini name="memory_limit" value="-1"/>
    </php>
    <groups>
        <exclude>
            <group>sanitychecks</group>
        </exclude>
    </groups>
</phpunit>

phpDoc parsing error from PHPStan

 ------ ----------------------------------------------------------------- 
  Line   src/SitemapPage.php                                              
 ------ ----------------------------------------------------------------- 
  48     PHPDoc tag @param has invalid value ($member): Unexpected token  
         "$member", expected TOKEN_IDENTIFIER at offset 18                
 ------ -----------------------------------------------------------------

Using this:
https://github.com/silbinarywolf/silverstripe-phpstan

@micschk
Copy link
Author

micschk commented Jul 16, 2018

@silbinarywolf made the requested changes, currently don't have a quick v4 based project to test on so hope you can run the tests again?

@silbinarywolf
Copy link

@micschk Still failing (after a few modifications to the codebase, as fixtures for the test still werent setup properly)

See here:
https://github.com/symbiote/silverstripe-sitemap/commits/fix-ss4-tests

Looks like getRootPages was either removed from SitemapPage or from core?

1) Symbiote\SitemapPage\Tests\SitemapPageTest::testShowAll
BadMethodCallException: Object->__call(): the method 'getRootPages' does not exist on 'Symbiote\SitemapPage\SitemapPage'

/shared/httpd/silverstripe-4-1-1/htdocs/vendor/silverstripe/framework/src/Core/CustomMethods.php:54
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/tests/SitemapPageTest.php:36
/shared/httpd/silverstripe-4-1-1/htdocs/vendor/phpunit/phpunit/phpunit:52

2) Symbiote\SitemapPage\Tests\SitemapPageTest::testShowChildrenOf
BadMethodCallException: Object->__call(): the method 'getRootPages' does not exist on 'Symbiote\SitemapPage\SitemapPage'

/shared/httpd/silverstripe-4-1-1/htdocs/vendor/silverstripe/framework/src/Core/CustomMethods.php:54
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/tests/SitemapPageTest.php:52
/shared/httpd/silverstripe-4-1-1/htdocs/vendor/phpunit/phpunit/phpunit:52

3) Symbiote\SitemapPage\Tests\SitemapPageTest::testShowSelected
BadMethodCallException: Object->__call(): the method 'getRootPages' does not exist on 'Symbiote\SitemapPage\SitemapPage'

/shared/httpd/silverstripe-4-1-1/htdocs/vendor/silverstripe/framework/src/Core/CustomMethods.php:54
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/tests/SitemapPageTest.php:73
/shared/httpd/silverstripe-4-1-1/htdocs/vendor/phpunit/phpunit/phpunit:52

4) Symbiote\SitemapPage\Tests\SitemapPageTest::testShowInMenusRespected
BadMethodCallException: Object->__call(): the method 'getRootPages' does not exist on 'Symbiote\SitemapPage\SitemapPage'

/shared/httpd/silverstripe-4-1-1/htdocs/vendor/silverstripe/framework/src/Core/CustomMethods.php:54
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/src/SitemapPage.php:93
/shared/httpd/silverstripe-4-1-1/htdocs/sitemap/tests/SitemapPageTest.php:82
/shared/httpd/silverstripe-4-1-1/htdocs/vendor/phpunit/phpunit/phpunit:52

@silbinarywolf
Copy link

@micschk Are you able to fix the problems mentioned above please?

@micschk
Copy link
Author

micschk commented Jul 23, 2018 via email

@silbinarywolf
Copy link

@micschk We're happy to wait if you're OK to do so in a month.

If we have any urgency on our side to get this done, we'll merge this in and cover off the remaining work.
Thanks for this!

@nglasl
Copy link

nglasl commented Oct 1, 2019

Hey @micschk, just wanted to check in where this may have got to?

@micschk
Copy link
Author

micschk commented Oct 7, 2019

Sorry, haven't made any progress on this...

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.

3 participants