[WIP] fixed generation of bbcode for uppercased tags#76
[WIP] fixed generation of bbcode for uppercased tags#76frastel wants to merge 1 commit intojbowens:masterfrom
Conversation
|
Ugh! Can you remove 2bd7dc7 and force push? This is extremely hard to assess correctly as-is. Appreciate your effort, though! |
| * @param string $code | ||
| * | ||
| * @dataProvider textCodeProviderWithInvalidCode | ||
| * @group debug |
frastel
force-pushed
the
fix/tag-normalization
branch
2 times, most recently
from
dc30728 to
6005b2c
Compare
|
@DaSourcerer Sure no problem. Branch is cleaned. |
| <?php | ||
| require_once dirname(__DIR__) . DIRECTORY_SEPARATOR . 'Parser.php'; No newline at end of file | ||
| require_once dirname(__DIR__) . DIRECTORY_SEPARATOR . 'Parser.php'; | ||
| require_once dirname(__DIR__) . DIRECTORY_SEPARATOR . 'UppercasedCodeDefinitionSet.php'; |
There was a problem hiding this comment.
Strange, I needed this for Travis. It worked fine without it on my local setup.
Can you elaborate a little on what problems that causes? |
|
@shmax I think, he doesn't experience any actual problems safe for the parts of the input that could not be parsed as tags being forcibly turned all-lowercase. I am actually in support of this, but find this implementation not very convincing. There has to be a better way than to keep tagname and the original token when one is directly derived from the other. Could be this is a bit of a hint some refactoring may be in order? After all, we've got Oh, and while we're at it: Do we want to support tagnames with multibyte characters in them? |
That was my thought as well, particularly as he's doing some run-time string operations that weren't there before, even with the double storage. |
JBBCode/ElementNode.php
Outdated
| foreach($this->attribute as $key => $value){ | ||
| if($key == $this->tagName){ | ||
| foreach ($this->attribute as $key => $value) { | ||
| if ($this->normalize($key) == $this->normalize($this->originalTagName)) { |
There was a problem hiding this comment.
can you check against $this->tagName? It's already been normalized by this point, hasn't it?
There was a problem hiding this comment.
I was worried about this change by myself too. I'll go for the strcasecmp approach.
I didn't expected any text modification when an invalid code is given. For my current project the strtolower modification is not accetable. @DaSourcerer The changes might not be very convincing however I didn't want to introduce too many changes at once in one of my first contributions to this project and thus I tried to not break the current structure. |
frastel
force-pushed
the
fix/tag-normalization
branch
from
6005b2c to
3007a04
Compare
Just curious: is it very difficult to implement compared to the current implementation? |
|
@frastel: As I said, this may be a hint some refactoring were in order. I understand your reluctance to introduce possibly code-breaking changes, but I just feel there is a better solution. Btw: That would be a shortcoming on our side, not yours ;) @Kubo2: It is so-so. We would introduce a hard dependency on the mb extensions (which is probably less probelmatic than I may make it sound like here). We also use the normalized (i.e. lowercased) tag names for various lookups. I am not quite sure how array keys with mb-characters in them behave. Worst scenario: We would lose all 𝓞(1) lookups and have them replaced with 𝓞(n). Worth it? I really don't know ... |
4 participants
This PR is actually based on my other cleanup PR. This PR makes only sense after the cleanup PR has been merged.
I found two issues:
[URL=INVALID]foo[/URL]was generated to[url=invalid]foo[/url].=. So[URL foo=bar]INVALID[/URL]was generated to[url= foo=bar]INVALID[/url].I am not quite sure if the change in
ElementNode::getAsBBCodefor the empty=is the best solution. Therefore this PR is still WIP. Any feedback appreciated!