Skip to content

Extension is activated even if set to false #9

Closed
ikwattro opened this Issue Feb 18, 2012 · 12 comments

2 participants

@ikwattro

Hi,

First of all thanks for your work.

I've integrated your wrapper into a Symfony Bundle.

I have an issue when specifying the extensions to be activated, if I set an extension to false in the array passed to the Sundown\Markdown constructor, the extension is still activated.

$enabled_extensions = array(
'autolink' => false);
$md = new Sundown\Markdown(new \Sundown\Render\HTML(), $enabled_extensions);
$var = "http://www.mysite.com";
echo $md->render($var);

This will still autolink the given var.

Grtz

@chobie chobie added a commit that closed this issue Feb 19, 2012
@chobie fixed #9 : check enabled extensions or render flags correctly.
SUNDOWN_HAS_EXTENSION macro doesn't care specified zval value. now it can check correctly.
bc50651
@chobie chobie closed this in bc50651 Feb 19, 2012
@chobie
Owner
chobie commented Feb 19, 2012

Thanks reporting the issue. i've just fixed this problem on development branch.
I'll bump up version when I added render flags tests.

@ikwattro

Thanks for the reply, unfortunately I'm still having the issue.

@chobie chobie reopened this Feb 19, 2012
@chobie
Owner
chobie commented Feb 19, 2012

hmm, i might missed something.

can you paste NO_INTERACTION=1 make test result?

fully instruction:

git clone https://github.com/chobie/php-sundown.git
cd php-sundown
rake submodule compile
NO_INTERACTION=1 make test

above commands will be compile latest php-sundown and execute test cases.

I've checked autolink behavior only at this time. please let me know more specifically what are you trying?

@ikwattro

The make test pass successfully. My app contain a $config array containing all the available extensions with true or false.

So I have an array like this :

protected $config = array(
        'no_intra_emphasis' => false,
        'tables' => true,
        'fenced_code_blocks' => true,
        'autolink' => true,
        'strikethrough' => true,
        'lax_html_blocks' => true,
        'space_after_headers' => true,
        'superscript' => false,
    );

If I pass this, all extensions set to false are not taken into account. I have to pass an array with only the true values.

@ikwattro

You can see here the complete class, I have two arrays (1 with the complete config, 1 with only the true values). The array to be passed in order to get it momently work is the $this->enabled_extensions array:

https://github.com/kwattro/KwattroMarkdownBundle/blob/master/Parser/Parser.php

@ikwattro

And here the paste of the make test command :


kwattro@kwattro-laptop:~/libs/php-sundown$ NO_INTERACTION=1 make test

Build complete.
Don't forget to run 'make test'.


=====================================================================
PHP         : /usr/bin/php 
PHP_SAPI    : cli
PHP_VERSION : 5.3.6-13ubuntu3.6
ZEND_VERSION: 2.3.0
PHP_OS      : Linux - Linux kwattro-laptop 3.0.0-16-generic #28-Ubuntu SMP Fri Jan 27 17:50:54 UTC 2012 i686
INI actual  : /home/kwattro/libs/php-sundown/tmp-php.ini
More .INIs  :  
CWD         : /home/kwattro/libs/php-sundown
Extra dirs  : 
VALGRIND    : Not used
=====================================================================
TIME START 2012-02-19 17:20:21
=====================================================================
PASS Check for sundown presence [tests/001.phpt] 
PASS Check for Sundown::__construct arguments [tests/002-basic-constructor-arguments.phpt] 
PASS Check for determine enabled extensions. [tests/002-basic-enabled-extensions.phpt] 
PASS Check for determine enabled extensions. [tests/002-basic-enabled-render-flags.phpt] 
PASS Check for Sundown::to_html() feature [tests/002-basic-to_html.phpt] 
PASS Check for Sundown::__construct arguments [tests/002-constructor-arguments.phpt] 
PASS Check for Sundown\Markdown::__construct() feature [tests/003-advanced-constructor.phpt] 
PASS Check for determine enabled extensions. [tests/003-advanced-enabled-extensions.phpt] 
PASS Check for determine enabled extensions. [tests/003-advanced-enabled-render-flags.phpt] 
PASS Check for Sundown\Render\HTML::block_code() feature [tests/003-advanced-render-html-block_code.phpt] 
PASS Check for Sundown\Render\HTML::block_html() feature [tests/003-advanced-render-html-block_html.phpt] 
PASS Check for Sundown\Render\HTML::block_quote() feature [tests/003-advanced-render-html-block_quote.phpt] 
PASS Check for Sundown\Render\HTML::header() feature [tests/003-advanced-render-html-header.phpt] 
PASS Check for Sundown\Render\HTML::list_box() feature [tests/003-advanced-render-html-list.phpt] 
PASS Check for Sundown\Render\HTML::paragraph() feature [tests/003-advanced-render-html-paragraph.phpt] 
PASS Check for Sundown::to_html() feature [tests/003-to_html.phpt] 
=====================================================================
TIME END 2012-02-19 17:20:22

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :    0
Exts tested     :   45
---------------------------------------------------------------------

Number of tests :   16                16
Tests skipped   :    0 (  0.0%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Expected fail   :    0 (  0.0%) (  0.0%)
Tests passed    :   16 (100.0%) (100.0%)
---------------------------------------------------------------------
Time taken      :    1 seconds
=====================================================================
@chobie
Owner
chobie commented Feb 19, 2012

Thank you quick response. test result looks fine.

for now, I've modified Acme\DemoBundle\DemoController::helloAction like this.

    public function helloAction($name)
    {
        $markdown = $this->container->get('kwattro_markdown');
        $string = "https://github.com/"; //Some string to transform

        return array('name' => $name,'md'=>$markdown->render($string, null, array("autolink"=>false)));
    }

It outputs following image. currently autolink extension is disabled. so that does not effect.

before this issue. sundown extension only checks specified hash key exist. but now,
that checks hash key and value is true. so you can put array without filtering.
I haven't added these test cases yet so i don't have confidence. but probably it works.
(Sometimes i misread the context as my English skill so poor. please correct me if i misread it)

i'm busy this two days. I might be slow response but i wanna fix this issue soon.

btw, there are little difference in Kwattro\MarkdownBundle\DependencyInjection\Configuration and Kwattro\MarkdownBundle\Parser\Parser.
Configuration specifies lax_html_blocks as false. but Parser specifies true.

@ikwattro

Yes, it works because it momently use my second array which unset all false values.

If you want the normal version, use branch beta1 instead.

I'll try to add some phpunit tests.

Thanks for the catch of the difference between config and parser.

Nice evening

@ikwattro

@chobie I've completed refactored my code. it seems that everything is working fine now.

just one thing i've seen today: it is not possible to change the renderer and the extensions configuration within the same instance. we have to create a new markdown instance if we want to change the renderer for eg. ? is it correct or i've missed something.

@ikwattro ikwattro closed this Feb 20, 2012
@chobie
Owner
chobie commented Feb 21, 2012

@kwattro Cheers!

Yes, currently that needs to create a new instance. but that's too much.
I'll add some methods that are able to modify render flags and extensions in this weekend.

@ikwattro

Ooooh nice, it would be a serious improvement.

By the way, I use your php extension of your work (http://pecl.php.net/package/sundown) for the Travis test of my bundle but this does not reflect your last changes. If you intend to push your changes to the php extension also please let me know it.

Thanks

@chobie
Owner
chobie commented Feb 21, 2012

hey, i've just updated pecl release. now, you can install v0.2.0 via pecl install sundown-beta.
i made mess commit logs but never mind that :'-(

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.