Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix init of the Markdown parser when use namespace #46

Closed
wants to merge 1 commit into from

4 participants

@wysman

Fix init of the Markdown parser when we add namespace at the top of the PHP file
Tested with PHP 5.3.10

To test, just add "namespace toto" at the top of the file. And call "\toto\Markdown" function from another.

@wysman wysman referenced this pull request
Closed

Allow use within Namespaces #41

@michelf
Owner

Ah, now I see what you meant in issue 41.

At first I though it'd be better if the namespace was added in the MARKDOWN_PARSER_CLASS define. The point of this define is so someone can set it to his custom parser class prior including markdown.php, which would result in a Markdown function calling that other parser. If you add the namespace in the function, the override won't work anymore (unless the class is in the same namespace). But then I realized that if you add a namespace at the top that override mechanism can't work anyway so perhaps it's useless whining on my part.

Currently PHP support goes back tp PHP 4. This patch would bring the requirements to PHP >= 5.3 (for namespace support), which is pretty drastic.

Now, I don't want to hold everyone back in time, but I'd rather have PHP Markdown not break backward compatibility if we can avoid it easily. I don't think keeping backward compatibility would be too costly: check for the PHP version before attempting to prefix the class name with a namespace (for PHP 5.2) and keep the constructors named after the class name (for PHP 4).

All this brings down the question: if you're going to edit the file to add a namespace declaration, can't you also change the class name in MARKDOWN_PARSER_CLASS at the same time? And why edit the file in the first place instead of just using the non-namespaced original file?

@wysman

Hi,

Currently PHP support goes back to PHP 4. This patch would bring the requirements to PHP >= 5.3 (for namespace support), which is pretty drastic.

I am not sure that create a code for PHP >= 5.3 (2 years old) is more drastic than be blocked on a 5 years old release with a discontinued support and a notice to upgrade to PHP 5.

And why edit the file in the first place instead of just using the non-namespaced original file?

I use a PHP Framework [1] where namespace is an important part in the code design. It's use by the autoload function to find the good file the load.

[1] http://www.photon-project.com/

@jnovack

I also am having issues with autoload support. The key difference being that the filename "markdown" is NOT the same as the class "Markdown_Parser". They MUST be equal for autoload to work. Underscores are reserved for subdirectories.

I recommend you change the class to Parser_Markdown and put the file as Parser/Markdown.php --OR-- change the class to Markdown and make the file name Markdown.php (diff below).

Autoload support allows you to dynamically load the class when needed. In larger projects, it's common to check all the dependencies (which autoload all the PEAR packages required in a setup.php, for example).

--- /usr/share/pear/markdown.php 2012-09-25 16:30:33.000000000 -0400
+++ /usr/share/pear/Markdown.php 2012-09-25 16:30:53.000000000 -0400
@@ -38,7 +38,7 @@

 ### Standard Function Interface ###

-@define( 'MARKDOWN_PARSER_CLASS',  'Markdown_Parser' );
+@define( 'MARKDOWN_PARSER_CLASS',  'Markdown' );

 function Markdown($text) {
 #
@@ -188,7 +188,7 @@
 # Markdown Parser Class
 #

-class Markdown_Parser {
+class Markdown {

  # Regex to match balanced [brackets].
  # Needed to insert a maximum bracked depth while converting to PHP.
@@ -215,7 +215,7 @@
  var $predef_titles = array();


- function Markdown_Parser() {
+ function Markdown() {
  #
  # Constructor function. Initialize appropriate member variables.
  #
@jnovack jnovack referenced this pull request in cweiske/phorkie
Merged

Fix SetupCheck / Indexing #11

@cweiske

jnovack, this is only necessary for PSR-0 compliance and autoloaders that expect such class layouts. You can write a different autoloader that makes loading the markdown class fine, although I think that following PSR-0 would be nice.

@michelf
Owner

Many people are using PHP Markdown in different ways. If I change the name of the class, I'll definitely break many people's code. It's the same thing with adding a namespace declaration at the top, which is pretty much akin to changing the name of the class (in addition to be incompatible with those using earlier PHP versions).

When I release a new version, many will replace PHP Markdown in their CMS by swapping the file for the new one. Some also replace PHP Markdown for PHP Markdown Extra, or the reverse, by downloading and replacing the file. Many of those people will feel completely lost if code has to be changed elsewhere in their CMS to make this work.

This is why I'm very conservative with any change affecting how Markdown must be used from the outside. @jnovack I don't intend to change the class names, at least not at this time. If you want autoloading to work, you can write a separate file that'll include markdown.php when it autoloads.

@jnovack

That is completely respectable and I fully understand. May I propose an alternative solution?

Would you kindly INCLUDE the PHP5 version in the project? You have a number of developers willing to help, and you maintain control and direction of the project.

This allows people who currently have and require your project to seamlessly upgrade to PHP5 and they gain the comfort and security of still using a well-established and respected repository and author.

@michelf
Owner

I already have two branches to maintain ('master' and 'extra' one). One thing that could be done is have a separate branch for a PHP 5.3 library-style version that'd include a namespace. I think I can keep it synced almost automatically using merges (if setup correctly), just like branches master and extra are merged right now. I just opened a new issue to discuss this, please let me know what you think.

@michelf
Owner

See the new lib branch, which has a namespace. ;-) Right now, it's pre-release. Eventually, this'll probably become the default branch.

@michelf michelf closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 19, 2012
  1. Fix init of the Markdown parser when we add namespace at the top of t…

    William MARTIN authored
    …he file
    
    Tested with PHP 5.3.10
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 6 deletions.
  1. +4 −6 markdown.php
View
10 markdown.php
@@ -56,7 +56,7 @@ function Markdown($text) {
# Setup static parser variable.
static $parser;
if (!isset($parser)) {
- $parser_class = MARKDOWN_PARSER_CLASS;
+ $parser_class = __NAMESPACE__ . '\\' . MARKDOWN_PARSER_CLASS;
$parser = new $parser_class;
}
@@ -238,8 +238,7 @@ class Markdown_Parser {
var $predef_urls = array();
var $predef_titles = array();
-
- function Markdown_Parser() {
+ public function __construct() {
#
# Constructor function. Initialize appropriate member variables.
#
@@ -1690,8 +1689,7 @@ class MarkdownExtra_Parser extends Markdown_Parser {
# Predefined abbreviations.
var $predef_abbr = array();
-
- function MarkdownExtra_Parser() {
+ public function __construct() {
#
# Constructor function. Initialize the parser object.
#
@@ -1717,7 +1715,7 @@ function MarkdownExtra_Parser() {
"doAbbreviations" => 70,
);
- parent::Markdown_Parser();
+ parent::__construct();
}
Something went wrong with that request. Please try again.