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

Jump to bottom

Use doctrine/static-website-generator #256

Merged
merged 1 commit into from Oct 25, 2018
Merged

Use doctrine/static-website-generator #256

merged 1 commit into from Oct 25, 2018

Conversation

jwage
Copy link
Member

@jwage jwage commented Oct 15, 2018 •
edited

Use https://github.com/doctrine/static-website-generator for the basic static site generator functionality.

@jwage jwage added Improvement Improvement to the code base WIP Something that is a work in progress. labels Oct 15, 2018
@jwage jwage force-pushed the use-routes branch 3 times, most recently from 2c3cb26 to 1221dc2 Compare October 15, 2018 21:34
SenseException
SenseException requested changes Oct 15, 2018
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the introduction of the routes and need to add further reviews later. 👍

lib/Routing/Router.php Outdated
'GET',
$url['host'],
$url['scheme'],
$url['port'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the port optional? This new version of the website doesn't allow doctrine.website.url without a port.

Notice: Undefined index: port in lib/Routing/Router.php on line 40

lib/Builder/SourceFileParametersFactory.php Outdated
) : SourceFileParameters {
$parameters = [];

if (preg_match('/^\s*(?:---[\s]*[\r\n]+)(.*?)(?:---[\s]*[\r\n]+)(.*?)$/s', $string, $matches) > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if-block should be in a private method and get a good name.

lib/Request/RequestCollectionProvider.php Outdated Show resolved Hide resolved
lib/Routing/Router.php Outdated
'GET',
$url['host'],
$url['scheme'],
$url['port'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the port optional? This new version of the website doesn't allow doctrine.website.url without a port.

@jwage jwage force-pushed the use-routes branch 16 times, most recently from a2c1e96 to fd4fe97 Compare October 16, 2018 21:32
@jwage jwage changed the title Use routes for mapping urls to controllers. Use doctrine/static-website-generator Oct 16, 2018
SenseException
SenseException requested changes Oct 18, 2018
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked through the files now. Please take a look at my comments.

composer.json Outdated
@@ -17,6 +17,7 @@
"doctrine/inflector": "^1.3",
"doctrine/rst-parser": "dev-master",
"doctrine/skeleton-mapper": "dev-master",
"doctrine/static-website-generator": "dev-master",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of dev-master are in here now. Will there be a release version for this before the PR gets closed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. Getting the standalone pieces integrated here was one of the last steps. I still want to upgrade doctrine/doctrine-website to be a Symfony Flex app which uses https://github.com/doctrine/DoctrineSkeletonMapperBundle and https://github.com/doctrine/DoctrineStaticWebsiteGeneratorBundle

lib/Commands/BuildDocsCommand.php Show resolved Hide resolved
lib/DataSources/BlogPosts.php Show resolved Hide resolved
templates/contributors-list.html.twig Show resolved Hide resolved
templates/maintainers-list.html.twig Show resolved Hide resolved
@jwage jwage force-pushed the use-routes branch 5 times, most recently from ef52d7f to 783c196 Compare October 22, 2018 16:36
@jwage jwage force-pushed the use-routes branch 2 times, most recently from a23ab3b to 73843b3 Compare October 22, 2018 22:57
@jwage jwage mentioned this pull request Oct 22, 2018
Merged
@jwage jwage force-pushed the use-routes branch 2 times, most recently from 15580b1 to 8d2c6d9 Compare October 23, 2018 16:19
@jwage jwage removed the WIP Something that is a work in progress. label Oct 24, 2018
SenseException
SenseException approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are done and I was able to build the website locally (excluding those sites that needed Github connection).

@jwage jwage merged commit c8b8d51 into master Oct 25, 2018
@jwage jwage deleted the use-routes branch October 25, 2018 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SenseException SenseException SenseException approved these changes

Assignees
No one assigned
Labels
Improvement Improvement to the code base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jwage @SenseException