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

Update quick-start.md #109

Open
wants to merge 1 commit into
base: 3.4.x
Choose a base branch
from
Open

Update quick-start.md #109

wants to merge 1 commit into from

Conversation

pbcGIS
Copy link
Contributor

@pbcGIS pbcGIS commented Feb 22, 2022

Currently the snippet illustrating modifications to application/module.config.php is incorrect relative to the module.config.php that has been downloaded withthe skeletonapplication asinstructed at the top of the tutorial. This leaves the novice tutorial user facing some not helpful php errors.

The modification included here shows the exact additions to the module.config.php in the context of the template file that the user starts with.

Thanks.

Q A
Documentation yes/no
Bugfix yes/no
BC Break yes/no
New Feature yes/no
RFC yes/no
QA yes/no

Description

Currently the snippet illustrating modifications to application/module.config.php is incorrect relative to the module.config.php that has been downloaded withthe skeletonapplication asinstructed at the top of the tutorial.  This leaves the novice tutorial user facing some not helpful php errors.    

The modification included here shows the exact additions to the module.config.php in the context of the template file that the user starts with.

Thanks.
Copy link
Contributor

@afilina afilina left a comment

Choose a reason for hiding this comment

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

I agree with keeping namespaces and imports from the original skeleton code. I added some suggestions for the rest.

Comment on lines +147 to +162
],
],
'view_manager' => [
'display_not_found_reason' => true,
'display_exceptions' => true,
'doctype' => 'HTML5',
'not_found_template' => 'error/404',
'exception_template' => 'error/index',
'template_map' => [
'layout/layout' => __DIR__ . '/../view/layout/layout.phtml',
'application/index/index' => __DIR__ . '/../view/application/index/index.phtml',
'error/404' => __DIR__ . '/../view/error/404.phtml',
'error/index' => __DIR__ . '/../view/error/index.phtml',
],
'template_path_stack' => [
__DIR__ . '/../view',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we need to repeat the entire content of the file here. It makes it very hard to zero-in on the part that is relevant to the explanation. Perhaps we can use // ... to indicate omissions.

Suggested change
],
],
'view_manager' => [
'display_not_found_reason' => true,
'display_exceptions' => true,
'doctype' => 'HTML5',
'not_found_template' => 'error/404',
'exception_template' => 'error/index',
'template_map' => [
'layout/layout' => __DIR__ . '/../view/layout/layout.phtml',
'application/index/index' => __DIR__ . '/../view/application/index/index.phtml',
'error/404' => __DIR__ . '/../view/error/404.phtml',
'error/index' => __DIR__ . '/../view/error/index.phtml',
],
'template_path_stack' => [
__DIR__ . '/../view',
// ...

Comment on lines +94 to +95
// Declaring our module's namespace here. Allows us to automatically reference classes in the
// src folder of our module with pathnames relative to the src folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that explanation of namespaces (core PHP functionality) belongs here.

Suggested change
// Declaring our module's namespace here. Allows us to automatically reference classes in the
// src folder of our module with pathnames relative to the src folder.

Copy link
Member

Choose a reason for hiding this comment

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

…that explanation of namespaces (core PHP functionality) belongs here.

Right, because this is a beginner's guide for laminas-mvc and not for PHP and OOP.

// src folder of our module with pathnames relative to the src folder.
namespace Application;

// Explicitly referencing these classes permits us to reference them by their names below.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that explanation of namespaces (core PHP functionality) belongs here.

Suggested change
// Explicitly referencing these classes permits us to reference them by their names below.

Copy link
Member

Choose a reason for hiding this comment

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

…and longer explanations do not belong in code examples.

Comment on lines +143 to +147
Controller\IndexController::class => InvokableFactory::class,
// Add the HelloController class to the array of invokable controllers.
Controller\HelloController::class => InvokableFactory::class,

],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just removing an unnecessary newline.

Suggested change
Controller\IndexController::class => InvokableFactory::class,
// Add the HelloController class to the array of invokable controllers.
Controller\HelloController::class => InvokableFactory::class,
],
Controller\IndexController::class => InvokableFactory::class,
// Add the HelloController class to the array of invokable controllers.
Controller\HelloController::class => InvokableFactory::class,
],

@@ -84,29 +84,82 @@ If you are at all uncertain about the source of a variable in your view, escape

Routes determine which controller to call based on the URI and other information from the request.

Configure a route and a controller in `module/Application/config/module.config.php`:
A couple of array values are declared for routes and controllers are added to `module/Application/config/module.config.php` as shown below:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that this rephrasing makes things more confusing. What problem are we attempting to solve in the original sentence?

Copy link
Member

Choose a reason for hiding this comment

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

It should be pointed out here that the existing module configuration file must be extended.


Excerpt from the documentation writer's guide (working draft):

Filenames

A filename must be placed before a code example because there are always two parts for this information:

  1. what kind of file is this
  2. and what is the name or path.

Both informations in a code comment would be too long and misplaced.

Examples

Create a file for the module configuration called module.config.php under laminas-mvc-tutorial/module/Album/config/:

…in a factory, e.g. src/Album/Controller/AlbumControllerFactory.php:

…in a module configuration file, e.g. module/Album/config/module.config.php:

'action' => 'world',
],
],
],
// end of New route insertion
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't use comments to visually delimit code sections.

Suggested change
// end of New route insertion

],
],
],
// New Route: used to generate links, among other things.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear whether this says that the 'hello-world' route is for links, or whether routes in general help with generating links. This information can possibly be elaborated on in the first paragraph under "Create a Route".

@pbcGIS
Copy link
Contributor Author

pbcGIS commented Feb 23, 2022

Hi Afalina, Thanks for taking a look at this. I appreciate your improvements.

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.x Quick-Start module.config snippet needs improvement.
3 participants