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

Feature/configuration factory #1597

Merged
merged 44 commits into from Oct 9, 2015

Conversation

rgeraads
Copy link
Member

The ConfigurationFactory that converts phpdocumento2 / phpdocumento3 xml to an array

@mvriel
Copy link
Member

mvriel commented Jun 30, 2015

Hi @rgeraads,

You're off to a good start! During the code review I have found the following points of improvements:

  • Can you please move the ConfigurationFactory to the \phpDocumentor namespace?
  • Can you move/use the xsd to/in the /data/xsd folder?
  • I am missing the replaceLocation() and get() method (see Class Diagram); it seems you renamed that to convert?
  • The XML is retrieved in the constructor; can you move that to the get() method and in the constructor just record the URI?

    This is because the configuration file may be overridden during execution of the application and we want to load the configuration at the latest moment, and that is in the get. Please keep in mind that the get may be called multiple times and building the configuration should not reoccur unless the URI has changed. This will save performance.

  • A significant amount of information is missing from the generated array for the phpDocumentor 2 XML; the output folder and source files are not set
  • Can you de-duplicate the defaults? I see the same defaults (such as default template) being used multiple times. My preferred option would be a private property 'defaults' that can also be overridden via the constructor
  • Can you make the xsd location configurable in the constructor? Since we could have multiple XSD files (one for each version) I think it would be best to add that to the defaults array

In addition to the above I will add a few comments inline with the source.

return [
'folder' => (string) $version->folder,
'api' => [
'format' => 'php',
Copy link
Member

Choose a reason for hiding this comment

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

this should also come from the xml

{
$this->validate($phpDocumentor);

$extensions = [];
Copy link
Member

Choose a reason for hiding this comment

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

the default for extensions is php, php3 and phtml; by omitting content here the parser won't pick up on any file

@mvriel
Copy link
Member

mvriel commented Aug 22, 2015

My apologies for the number of points; I noticed some things that would quite possibly lead to issues later on.

mvriel added a commit that referenced this pull request Oct 9, 2015
@mvriel mvriel merged commit 845e7f3 into phpDocumentor:develop Oct 9, 2015
@rgeraads rgeraads deleted the feature/configuration-factory branch October 10, 2015 18:24
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.

None yet

3 participants