Skip to content

Conversation

@Kirill
Copy link
Member

@Kirill Kirill commented Mar 18, 2019

for master and version 2.1.x

for master and version 2.1.x
@Kirill Kirill requested a review from bkraul March 18, 2019 21:37
@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

@Kirill , I really have not tested this with PHP 5.6. Are we positive the new version does not work with it? Do you think we should look into making it compatible? Or should we just drop off and let users stay with the older versions and encourage them to migrate to PHP 7+?

@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

I will be doing some testing soon on this. Thanks for the PR.

@Kirill
Copy link
Member Author

Kirill commented Mar 18, 2019

I can't test it for older version. I look on README.

@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

Also, as per the guide, please try to make your changes on develop or feat branches rather than on your fork's master, because then you will have a hard time keeping your fork's master updated from the main master. It goes like this:

Kirill/BBCodePlus:master (1 : 1 with mantisbt-plugins/BBCodePlus:master)
|
|
\
  \
    \
      -- Create branch Kirill/BBCodePlus:develop
             |
             |
             ------ PR of develop into mantisplugins/BBCodePlus:master
                                            |
                                            |
                               PR is merged in mantisbt-plugins/BBCodePlus:master
                                            |
                                            |
                                            |
                   Kirill pulls mantisbt-plugins/BBCode:master into Kirill/BBCodePlus:master
                                            |
Kirill/BBCodePlus:master---------------------
|
|
| __ master always in sync with mantisplugins/BBCode:master

@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

I see what you mean about the BBCode parser being only for PHP 7.1+. Hmmm...yeah, then I guess we need to stay with 7.1+ and tell users that BBCode 2.1.x requires PHP 7.1+. PHP 5.6 is EOL already, the security fixes stopped Jan 1, 2019 (according to PHP.net).

@Kirill
Copy link
Member Author

Kirill commented Mar 18, 2019

PHP 5.6 is EOL already, the security fixes stopped Jan 1, 2019 (according to PHP.net).
Yes, but master branch or MantisBT require PHP 5.5.9+

@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

I will test older PHP soon. It might not be as bad. We'll see. The parsers are really simple.

@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

I think the author is requiring PHP 7.1 +, because hIs libraries are primarily designed to work in a Laravel environment. But they can also be used as a standalone classes.

@bkraul
Copy link
Member

bkraul commented Mar 18, 2019

So, I found that it was indeed incompatible with PHP 5.6. I have made modifications to the parser class, and now it is compatible with PHP 5.6. I will test with PHP 5.5.9 to make sure. If it all works, if you don't mind, I will go ahead and close this PR. I will also post a better Git graphic to the guide explaining the development and PR model. I am sorry the "diagram" I made here is not very good.

@bkraul
Copy link
Member

bkraul commented Mar 19, 2019

@Kirill, I can confirm that the changes I have made to the parser classes work on PHP 5.5.9+. I will go ahead and close this PR and send the changes in another PR. As I said before, let's get in the habit of not accepting PRs that come from a fork's master branch so as to avoid regressions and commit conflicts. I will try to clarify this in the guide more.

After I merge the PR, please fetch the latest upstream into your fork's master add then do a full git reset --hard upstream master on your fork's master (assuming you have set up the upstream) to remove the latest commit on your fork, then push-force to your origin/master. This will bring your fork's master back in sync with the main project repo.

@bkraul
Copy link
Member

bkraul commented Mar 19, 2019

Fixes for compatibility have been applied on PR #43. This PR is now closed.

@bkraul bkraul closed this Mar 19, 2019
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.

2 participants