Skip to content

eZ Publish 4.3+ Template compatibility and documentation improvements #1

Closed
wants to merge 4 commits into from

2 participants

@brookinsconsulting

This pull request consists of 3 simple commits to the documentation, settings and module view php files.

The pull request provides enhanced documentation in GitHub markup (The new favorite), utf8 settings formatting consistency and module view php source code comment enhancements.

These enhancements make the code better and new user documentation more friendly.

Thank you for your continued support

Cheers,
Brookins Consulting

@brookinsconsulting brookinsconsulting Updated: Added utf8 formatting and proper line endings b32dc36
@brookinsconsulting brookinsconsulting Added: Refactor documentation extensively to use GitHub Markup. Re-or…
…ganized extension documentation. Added copy of the license.
eaa0963
@brookinsconsulting brookinsconsulting Updated: Replaced templateInit includes and calls with eZTemplate::fa…
…ctory to be compatible with ez 4.3+ (4.4/4.7/CB2012.02). Added phpdoc comment block documentation and copyright attribution
6411eb8
@arnebratt arnebratt commented on the diff Apr 8, 2012
doc/COPYRIGHT.md
@@ -0,0 +1,15 @@
+Copyright (C) 2010 - 2012 A.Bakkeboe.
@arnebratt
Owner
arnebratt added a note Apr 8, 2012

Is this file needed? Try not to repeat documentation information over multiple files, that just makes it harder to update.

I would also remove the same information from the README.md file, having a doc/LICENSE file should cover the needs here.

We at BC have patterns we apply to extension copyright documentation. One of them is the existence of a clear copyright assignment file with clear documentation on the license of the copywritten works, outside of the ezinfo and extension.xml standards. This file provides for both of these concerns very well for other projects, we think each extension should have one.

We think having doc/LICENSE is simply not enough as that is mearly license distribution not assignment, though again these are BC standards. A README.md is often considered enough but we believe it is better to be more clear than enough.

Duplication here is minimal and would change very infrequently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnebratt arnebratt commented on the diff Apr 8, 2012
doc/INSTALL.md
+* Recently tested on eZ Publish Community Build 2012.02
+* Previously tested on eZ Publish 4.1, 4.2 and 4.3
+
+AdminAid is a well written extension and should work normally on most versions of eZ Publish.
+
+
+o Installation
+
+Extract extension to the extension/adminaid/ directory.
+
+Activate the new extension by adding it in
+settings/override/site.ini.append.php like this:
+
+[ExtensionSettings]
+ActiveExtensions[]=adminaid
+
@arnebratt
Owner
arnebratt added a note Apr 8, 2012

I suggest making a note to have a look at the settings/aid.ini.append.php file here also.

We will prepare the changes you request here shortly

Hello!

We have added the note you requested (above) in the following git commit, brookinsconsulting@deee634

Thank you for your continued support!

Cheers,
Brookins Consulting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnebratt arnebratt commented on the diff Apr 8, 2012
doc/LICENSE
+PROGRAM PROVE DEFECTIVE, YOU ASSUME THE COST OF ALL NECESSARY SERVICING,
+REPAIR OR CORRECTION.
+
+ 12. IN NO EVENT UNLESS REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING
+WILL ANY COPYRIGHT HOLDER, OR ANY OTHER PARTY WHO MAY MODIFY AND/OR
+REDISTRIBUTE THE PROGRAM AS PERMITTED ABOVE, BE LIABLE TO YOU FOR DAMAGES,
+INCLUDING ANY GENERAL, SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES ARISING
+OUT OF THE USE OR INABILITY TO USE THE PROGRAM (INCLUDING BUT NOT LIMITED
+TO LOSS OF DATA OR DATA BEING RENDERED INACCURATE OR LOSSES SUSTAINED BY
+YOU OR THIRD PARTIES OR A FAILURE OF THE PROGRAM TO OPERATE WITH ANY OTHER
+PROGRAMS), EVEN IF SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF THE
+POSSIBILITY OF SUCH DAMAGES.
+
+ END OF TERMS AND CONDITIONS
+
+ How to Apply These Terms to Your New Programs
@arnebratt
Owner
arnebratt added a note Apr 8, 2012

Why is the section on "How to Apply These Terms to Your New Programs" included in the extension? Seems kinda meaningless to me?

This is the default LICENSE file as distributed with all software under this license. You should -not- modify the license text. See this mirror of the original license text for a reference: http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnebratt arnebratt commented on the diff Apr 8, 2012
doc/INSTALL.md
@@ -0,0 +1,45 @@
+AdminAid Install
+=============
+
+o What is AdminAid
@arnebratt
Owner
arnebratt added a note Apr 8, 2012

This section does not need to be here. It should be only in the README.md file, to make it simple to keep updated.

This is a BC standard to explain in the two files users read first what the extension is as clearly and briefly as possible at the top of the README.md and INSTALL.md

We think it helps the user to have a more well rounded set of documentation to use to remind and teach them.

We can remove this duplication but we think it's a better idea to leave it in. Besides the documentation changes so infrequently and you never really know which file the user will read first, README or INSTALL. This why we use this pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnebratt arnebratt commented on the diff Apr 8, 2012
modules/aid/class_attribute_select.php
$http = eZHTTPTool::instance();
+
+/**
+ * Define module view template
+ */
+$tpl = eZTemplate::factory();
@arnebratt
Owner
arnebratt added a note Apr 8, 2012

Why do we want to change to the new template factory? Is there any advantages to that as opposed to including the old template? I do not wish to stop supporting older eZ Publish versions if there is nothing to gain from it.

You have a very valid point. It looks more and more like eZ Publish 5.x will not use eZ Components and so there seems to be no reason to even bother with the core goal of this pull request as it is not a real eZ Publish requirement per say ... We saw a recommendation in debug that the extension should upgrade to the latest API supported today in CB2012.03 aka eZTemplate::factory() and so we created a helpful pull request to address that concern and try to help improve the documentation at the same time.

Hello!

After some consideration and careful watching of other eZ Publish Community Builds and Developers and after reviewing git branch support ...

We think it would be best to create a git branch of AdminAid's current eZ Publish 4.x Support now rather than later.

With the remarkable ease of use of git branches you can provide better eZ Publish API Support with your extension repository.

Also we think think after you accept this pull request it would also be a good idea to create a git branch of AdminAid's more recent eZ Publish 4.3+ Support.

When the next version of eZ Publish is released to the public you will still have the option to create another branch to address template support as necessary.

Thank you for your continued support! Please let us know how this finds you.

Cheers,
Brookins Consulting

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

In general I agree with the changes here. I would be more carefull with commenting everything though, adding a comment for every line on simple tasks like the template factory for instance just makes a lot more source code and gives no added documentation value. This goes double when you use three lines for every comment instead of one.

@brookinsconsulting

Thank you for your review and feedback. I will try to refactor the changes further and submit more changes soon based on your feedback.

@brookinsconsulting

In general I agree with the changes here. I would be more carefull with commenting everything though, adding a comment for every line on simple tasks like the template factory for instance just makes a lot more source code and gives no added documentation value. This goes double when you use three lines for every comment instead of one.

Hello!

In response to your comment after some thought a number of things come to mind.

BC Standards have long insisted that code with verbose comments that explain to all developers regardless of experience just what is happening as much as possible are much better than code without such verbose comments.

Also BC Standards have recently been improved to ensure that while lower level implementations of our standards (which did provide for a // comment standard) are still supported, they are discouraged in favor for the more recently popular comment block style comments:

/**

  • Comment about the next code block */

Please try to understand. Your users will respect the care we have taken to ensure the code is prepared for even more improved code block comment text if the current simpler text is not quite enough because their modified versions will be simpler to change the comments to meet their own code documentation needs simply by using code which provides for such blocks by default.

Providing for standard code block documentation, even simple or trivial documentation increases code value especially when the code's very use will always be in such a way that the additional length of the code in question is virtually trivially negated with the value provided (re: inline code documentation blocks) in exchange for said length of code. Also the longer code block standard is also similar to the more recent / popular / preferred documentation standard known as PHPDoc.

Please let us know how these replies find you.

Respectfully,
Brookins Consulting

@brookinsconsulting

Since this extension pull request was not understood, accepted or welcomed it has now been closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.