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

Pass string as method argument #154

Open
dereuromark opened this issue Sep 18, 2020 · 3 comments
Open

Pass string as method argument #154

dereuromark opened this issue Sep 18, 2020 · 3 comments
Labels
major major (BC break)

Comments

@dereuromark
Copy link
Collaborator

dereuromark commented Sep 18, 2020

Is there a reason the string has to be put into the constructor, and the actual config happens at runtime?
It makes it difficult for DI and libraries/config to adjust the object ones in constructing and then using it.

If you look into e.g.
https://commonmark.thephpleague.com/1.5/extensions/github-flavored-markdown/
or https://github.com/kzykhys/Ciconia/blob/master/src/Ciconia/Ciconia.php#L42

Then you usually make the builder itself stateless and instead wrap statefulness internally.

$env = .. // Can contain all the filters and stuff
$options =  [
    'xhtmlOutput' => true,
    'strictMode' => false,
    'escapeHtml' => true
];

// We can pass defaults here if we want
$instance = new Decoda($env, $options);

// We can also set options here per convertion
$html = $instance->convert($bbcodeText, $options);
$html2 = $instance->convert($bbcodeText2, $otherOptions);

What do you think?

There will also be no need for reset() then as it keeps the wrapper stateless and reuses the objects here.

@dereuromark
Copy link
Collaborator Author

It also made the implementation here a bit more difficult: https://github.com/dereuromark/cakephp-markup/pull/5/files#diff-bceef8e25512a4d6ae70ac8dad7cf2a7R73
And effectively without being able to cache the stateless filters/hooks

@milesj
Copy link
Owner

milesj commented Sep 19, 2020

No strong reason that I remember. If I were to rewrite this now, I would most likely move the string to parse(), so that the same instance can be reused.

@dereuromark dereuromark added the major major (BC break) label Sep 19, 2020
@alquerci
Copy link
Contributor

@dereuromark On the Symfony bundle the pattern used is the factory one.

See https://github.com/helios-ag/FMBbCodeBundle/blob/350d7c0/Decoda/DecodaManager.php#L96
Usage https://github.com/helios-ag/FMBbCodeBundle/blob/350d7c0a01ac3ab057df836966fbd6c155721af7/Templating/BbcodeExtension.php#L60

Maybe this can help you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major major (BC break)
Projects
None yet
Development

No branches or pull requests

3 participants