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

Fix Issue #358 – preventing double nested links #433

Merged
merged 6 commits into from
Feb 28, 2018

Conversation

aidantwoods
Copy link
Collaborator

@aidantwoods aidantwoods commented Oct 1, 2016

  1. Add the ability for a parsed element to enforce that the line handler not parse any (immediate) child elements of a specified type.
  2. Use 1. to allow parsed Link elements to tell the line handler not to parse any child Links or Urls where they are immediate children.

1. Add the ability for a parsed element to enforce that the line handler not parse any (immediate) child elements of a specified type.
2. Use 1. to allow parsed Url elements to tell the line handler not to parse any child Links or Urls where they are immediate children.
@aidantwoods
Copy link
Collaborator Author

aidantwoods commented Oct 1, 2016

If we like 1. as a feature, then I can probably extend the functionality such that (disallowed) nesting can be asserted past a depth of one.

i.e. So although this PR fixes the original issue, cases such as:

[http://example.com](http://example.com)

It won't fix cases such as:
The following case is now also addressed in the second patch:

[*http://example.com*](http://example.com)

without being able to assert further than an immediate depth.

This is because the link can only currently assert nested exclusions for direct children. In the second example, the direct child of the Link is an italic element, and the inline Url is a child of the italic element (child of depth 2 relative to the Link).

I've built on the functionality of feature 1. in the previous commit to allow non nestables to be asserted indefinitely, or to a specified depth.
@aidantwoods
Copy link
Collaborator Author

aidantwoods commented Oct 2, 2016

I've built a little on the functionality as hinted in the above comment.

Inside the element array, one can now specify:
'non_nestables' => array('Url', 'Link'),
or
'non_nestables' => array(['Url'], ['Link']),
or mixed:
'non_nestables' => array('Url', ['Link']),
or the identical:
'non_nestables' => array('Url', array('Link')),

If you wish to only assert non nesting depth a finite number of times simply use the array syntax, and specify the depth assertion as the second array value.

e.g. the following will assert Link or Url may not be nested within a Link, at any depth, but Code may be inserted if greater than depth two:
screen shot 2016-10-02 at 17 32 33

So for the following data:
screen shot 2016-10-02 at 17 32 43

We achieve the following output:
screen shot 2016-10-02 at 17 32 57

Where inline Url is not ever allowed, but code is allowed past a depth of two

This commit serves to add comments detailing parts of the new functionality, and to adjust syntax preferences to match that of the surrounding document. The commit title also now reflects the most significant change made.
@aidantwoods
Copy link
Collaborator Author

@PhrozenByte @erusev any thoughts on the latest changes in this PR?

Just noticed another duplicate issue (#432) report popped up for this right before my PR, so looks like this fix is a much needed update

@PhrozenByte
Copy link
Contributor

PhrozenByte commented Oct 4, 2016

All in all: Looks great to me! 👍

However, I don't see a use case for specifying a maximum depth for non nestable types. I suggest removing the "array syntax" (i.e. 'non_nestables' => array('Url', 'Link') is the only supported syntax) and assuming that non nestable types are in effect for an infinite depth.

Removed granularity controls – elements are assumed to be non nestable indefinitely once declared.
@aidantwoods
Copy link
Collaborator Author

@PhrozenByte That makes sense to me!
Yeah, I couldn't immediately think of any useful cases for max nesting either – was an easy add on with the inheritance change though.

I've removed it for now, but if it's ever useful then it's easy to re-add if it's ever needed (and forward compatible with the current string syntax etc..)

It does simplify the code considerably not having the max depth and mixed syntax ;-)

{
$Inline['element']['non_nestables'][] = $non_nestable;
}
$Inline['element']['non_nestables'][] = $non_nestable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about $Inline['element']['non_nestables'] = array_merge($Inline['element']['non_nestables'], $non_nestable); instead of the foreach (foreach is slightly slower btw, however, not to such an extent that it makes any difference)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PhrozenByte I did consider that, though there is no guarantee that the $Inline['element']['non_nestables'] will exist – so the code suffers a little for readability with that added check IMO. Though I appreciate that's a subjective decision, so I'm happy to favour array_merge if you disagree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't think about that. I agree that adding another check doesn't improve readability, so it's probably best to keep it the way you did 😃

@@ -1003,6 +1003,13 @@ public function line($text)

foreach ($this->InlineTypes[$marker] as $inlineType)
{
# check to see if the current inline type is nestable in the current context

if (in_array($inlineType, $non_nestables))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if (!empty($non_nestables) && in_array($inlineType, $non_nestables))? in_array() really is very expensive, even with empty arrays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds sensible, I'll commit that change when I get a sec :)

Check if array is empty to shave some performance hits in the case than no non nestables are present.
@aidantwoods
Copy link
Collaborator Author

@PhrozenByte I think that's everything in the latest commit?

@PhrozenByte
Copy link
Contributor

Yes, perfect 👍

@aidantwoods
Copy link
Collaborator Author

@erusev guessing as before, you'll want camel casing instead of '_' as the word delimiter?
Anything else that needs adjusting?

@erusev
Copy link
Owner

erusev commented Oct 8, 2016

@aidantwoods

Yes, pls.

->line('abc *abc*', 'em') would produce abc *abc* and not parse the emphasis, right?

@aidantwoods
Copy link
Collaborator Author

@erusev almost – it would be ->line('abc *abc*', array('em')) or ->line('abc *abc*', ['em']), which would produce abc *abc* with no emphasis.

This is because it's built to allow multiple types to be disallowed, (including where multiple non nestables are aggregated over nested elements with different non nestable conditions). So line does have to be able to accept an array for that to function.

Swap `under_scores` for `camelCasing`
@erusev
Copy link
Owner

erusev commented Oct 8, 2016

@aidantwoods

Right, well, then there might be a better name for the parameter.

Non nestables seems to scary of a word for a parameter that just lists the types that the method should skip.

@aidantwoods
Copy link
Collaborator Author

@erusev perhaps, any suggestions? 'skipInlineTypes', 'ignoredInlineTypes'?

@erusev
Copy link
Owner

erusev commented Oct 8, 2016

Something like this.

Let me have a better look at the code later next week and I might come up with some ideas.

@hatrena
Copy link

hatrena commented May 18, 2017

Any chance to merge this PR ?

@erusev
Copy link
Owner

erusev commented May 18, 2017

@hatrena Sure, I'll try to review it by the end of the month.

@aidantwoods aidantwoods mentioned this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants