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

changed PageContent.php to accept nested includes #4192

Merged

Conversation

jasonF1000
Copy link
Contributor

tested and it works.
We have implemented levels depths that can be configured as needed. ( via includeDepth variable)
This solves issue #2845

@jasonF1000
Copy link
Contributor Author

hi @ssddanbrown, could you please review this pull request?

Thanks in Advance.

@ssddanbrown
Copy link
Member

ssddanbrown commented Apr 24, 2023

Thanks @jasonF1000 for offering this PR. A few of things:

  • The code provided seems to have whitespace issues, mainly in that it seems to be using non-breaking spaces instead of normal spaces within the indentation, which breaks this as code.
  • This loop only needs to be around parsePageIncludes, not blankPageIncludes too.
  • Ideally I'd want to have testing added to cover this scenario (With some depth checking to ensure this sticks to three levels max).

There are existing tests to cover page includes in this file:
https://github.com/BookStackApp/BookStack/blob/development/tests/Entity/PageContentTest.php
Would you be comfortable in adding the requested testing?

Note-to-self

If merged, this will need to be documented as a breaking change.

This loop is now only around parsePageIncludes and bugfixes the space indentation.
@jasonF1000
Copy link
Contributor Author

Hi @ssddanbrown i followed your recommendation and fixed the space issue and change the loop that it is only around the parsePageIncludes.

But i feel not so comfortable to add the testings, therefore i have to low experience with PHP. Sorry :(

@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Apr 24, 2023
@ssddanbrown ssddanbrown merged commit 011800d into BookStackApp:development Apr 27, 2023
@ssddanbrown
Copy link
Member

Thanks @jasonF1000, now merged for next release.
I followed with up with 56f234d to adjust formatting and add testing.

@jasonF1000
Copy link
Contributor Author

@ssddanbrown thank you for taking over the testing part and for the implementation of the feature :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants