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

Where is highlight.php v10.x/v11.x? #73

Open
allejo opened this issue May 3, 2020 · 26 comments
Open

Where is highlight.php v10.x/v11.x? #73

allejo opened this issue May 3, 2020 · 26 comments

Comments

@allejo
Copy link
Collaborator

allejo commented May 3, 2020

For anyone curious and before anyone asks, highlight.php 10.x/11.x is planned and development will start in the coming weeks. I first need to finish off my semester and a few other commitments.

highlight.php 10 will require PHP 5.6+ (see #55)

The current estimated release of highlight.php v10/11 is Winter 2021


Why the delay?

highlight.js v10 introduced some breaking changes where grammars can no longer be represented by just JSON; grammars now support JS callbacks. While not many grammars are using this feature as of yet, I'm hesitant to dedicate myself to manually porting JS to PHP.

Does this mean highlight.php is dead?

No. Not at all. I do have some ideas for automatically translating JS to PHP to address this problem. I simply have not had time to work on this.

It's been over a year since this issue was created. Why haven't you done it yet?

I've had other projects, work, and school that have required my attention/focus/time. Feel free to sponsor me to work on this project 😄 ❤️

@allejo allejo pinned this issue May 3, 2020
@joshgoebel
Copy link

joshgoebel commented May 3, 2020

Very curious to see how you handle the dynamism that's possible now.

@allejo
Copy link
Collaborator Author

allejo commented May 3, 2020

Very curious to see how you handle the dynamism that's possible now.

Me too 😅

@joshgoebel
Copy link

I used it in the new SHEBANG mode too since now it's actually possible (for the first time) to check if a match starts at the beginning of the content, not just the beginning of the line.

@phpfui
Copy link
Contributor

phpfui commented Sep 15, 2020

For version 10, can we change the HighlightUtilities namespace to Hightlight\Utilities and make Highlight\Utilities a static class with the functions as static methods? I think this is a better organization. No reason for two namespaces.

Also I think there should be a src directory to make the package a bit more standard.

Neither of these changes should really impact many users and it should be an easy change.

Happy to submit a PR with the above changes, I just don't like submitting PRs with changes such as this without buy in from the author. But V10 would be the time to make the change.

@allejo
Copy link
Collaborator Author

allejo commented Sep 15, 2020

For version 10, can we change the HighlightUtilities namespace to Hightlight\Utilities and make Highlight\Utilities a static class with the functions as static methods? I think this is a better organization. No reason for two namespaces.

The only reason I opted to have a separate namespace was so we can keep the Highlight namespace as loyal as possible to a port of the highlight.js project. Let me sit on this one for a bit.

Is there an advantage to having these functions under a static class? As functions, you can currently import them like use function \HighlightUtilities\getStyleSheetFolder() and even give them aliases. This is a 5.6+ feature, so considering the min requirement of 5.6 for 10.x, this would continue to work.

Also I think there should be a src directory to make the package a bit more standard.

Neither of these changes should really impact many users and it should be an easy change.

Happy to submit a PR with the above changes, I just don't like submitting PRs with changes such as this without buy in from the author. But V10 would be the time to make the change.

Yea, I've been wanting to change this project into a PSR-4 setup for a while, just never got around to it. Sure you can make that PR, continue to keep two separate namespaces for now though for Highlight and HighlightUtilities.

@phpfui
Copy link
Contributor

phpfui commented Sep 15, 2020

Actually after I wrote this, I realized you only need one namespace. You put in a class called Utilities with static methods. So instead of \HighlightUtilities\getStyleSheetFolder(), it becomes \Highlight\Utilities::getStyleSheetFolder().

It is a very easy port, just search and replace really and uses one namespace.

As for PSR-4, not sure it makes much of a difference vs PSR-0 if you put things in a src directory. The problem with PSR-0 and no source directory is that you can't programatically tell what belongs to the library and what is just support code. This may not matter to most users, but if you are using a super fast autoloader, then you need a mapping between the namespace and the file system, and you have no easy way to populate the file system without including a bunch of junk. Anyway, a src is best practice and does not really impact anything.

Let me know your thoughts on the Utilities class and static methods. I think it is the best approach.

Thanks thanks for responding. I use this package for my PHPFUI website to display the source and it works great, although sometimes a bit slow on larger files.

@phpfui
Copy link
Contributor

phpfui commented Sep 16, 2020

So I started moving to PSR-4 and replacing the HighlightUtilities namespace. Turns out you just change \HightlightUtilities\ to \Highlight\Utilities:: and you are done!

But I noticed that the project is PHP 5.4 (barf!). How about PHP 7.1 or 7.2? 7.1 is no longer supported, but a decent version. 7.2 adds nullable types, which may not be needed. Leave V9 for older versions, and V10 would be 7.1 or higher.

Also would want to upgrade PHPUnit, which is fairly easy.

@allejo
Copy link
Collaborator Author

allejo commented Sep 17, 2020

Before we get a bit too carried away...

I'm not really sold on the benefits of moving the HightlightUtilities namespace to an abstract class, \Highlight\Utilities. I really do not want to deviate too much from origin project (highlight.js) and clutter up the main Highlight namespace, which is why it was added as a separate one. What if highlight.js introduces a Utilities down the line? I want to make the separation of the components of this project clear, what is loyal to highlight.js, and what isn't.

As for the PHP version, v10 is currently planning on targeting 5.6; see the first post in this issue and issue #55. Since I'm actively working on a WordPress plugin that incorporates this library and there are no critical features that we need from PHP 7.x, I have no problems supporting 5.6 for a while longer. highlight.js is planning to have a quicker release schedule, so there'll be more opportunities to bump the minimum PHP version without having to wait for a major bump.

@phpfui
Copy link
Contributor

phpfui commented Sep 17, 2020

It is your library. I am simply suggesting some common techniques that work well. Classes with static methods work better than functions in namespaces as it allows for a simpler autoloader. When you start getting into larger projects, autoloader speed becomes an issue. A PSR-0 autoloader is super fast. Well structured PSR-4 libraries can be converted to PSR-0 easily. My autoloader takes 0 bytes of memory and has extremely low computational overhead (no array lookups). But it does not work well with namespaced functions (looking at you GuzzleHttp).

I am not big fan of taking more namespaces than needed, and you really only need one. I did delete the Autoloader class, so as far as classes in the Highlighter namespace that are not in the JS version, it is a wash. It is easy enough to rename Utilities if it comes to that.

The alternative might be to do a static method class in HighlightUtilities, but then you start duplicating names (ie. \HightlightUtilities\Utilities::methodName).

The biggest improvement is the src file structure, That will work with PSR-0 or 4.

Also the current standard for PHP libraries (Laravel and PHPUnit for example) is to just support current versions of PHP (meaning 7.2 currently). This encourages people to continue to upgrade, which benefits the entire community. Typed method parameters, typed return values, type properties and nullable types all help to insure your code works as expected. Typeless languages are simply not as good as more strongly typed languages. There are reasons why languages have historically added more and more type checking as the language progresses. FORTRAN, C, PHP and even the lowly JavaScript have all added more type checking as they evolved. But 5.6 is better than 5.3 for sure.

Let me know what you want to do and happy to update the PR. My biggest things are a src directory and no namespaced functions. PHP 7.2 is just me trying to push the PHP community into the 1990s of computer language design. I personally plan to bump the minimum PHP requirements in all my PHP projects to only currently supported versions when PHP 8 ships. Keeping up with PHP releases provides huge speed improvements, and I have seen noticeable speed improvements when ever I upgrade PHP. YMMV.

And thanks for providing this library!

@allejo allejo changed the title Where is highlight.php v10.x? Where is highlight.php v10.x/v11.x? May 16, 2021
@localheinz
Copy link

@allejo

Is there anything we can do to support the upgrade?

Any help needed on the code, or perhaps some sponsoring?

@phpfui
Copy link
Contributor

phpfui commented Jun 3, 2021 via email

@phpfui
Copy link
Contributor

phpfui commented Jun 4, 2021

I upgraded phpfui.com to dev-master and WOW! It is much faster. The largest file timed out before (170K) and everything seems super snappy now. I would recommend an immediate release of V10. It was plug in compatible. I did not have to change anything.

Here are some example pages to check out. The second one did not load before:

You are looking at the actual code that is running on the site. The site renders the documentation real time, it is not stored, but generated on request.

Also, if you want to include a full class documentation link in the readme file, use this link: http://phpfui.com/?n=Highlight It includes all the namespaces, classes and readme. Since PHPFUI uses this package, it will always be documented on PHPFUI, as it always documents the classes it uses at a minimum (plus some other libraries like PhpParser, which is totally awesome).

@joshgoebel
Copy link

I upgraded phpfui.com to dev-master and WOW! It is much faster.

Are you simply referring the the latest highlight.php available from GitHub?

@allejo Did you make a release with that huge speed fix a while back? If not prolly should.

@phpfui
Copy link
Contributor

phpfui commented Jun 4, 2021

Yes. I specified dev-master in composer.json. Was previously using v9.18.1.6. You can check out the commit here: phpfui/PHPFUIWebsite@b19a868#diff-f37acfaa6b11f575a9a6f41a75fa73a61d0f8ebc2c9b8cddc215d8aca10e44f5

I believe there was a huge speed fix in a regex, but think it is only in 10. My experiment would confirm that.

I think 10 should be released ASAP.

@joshgoebel
Copy link

I believe there was a huge speed fix in a regex, but think it is only in 10.

Indeed, but that's not what you're talking about if you're using just the master branch. There is no v10 code here yet. There was a BIG speed fix a while back, perhaps a release was just never cut. In which case it should be, but it would be another v9 release, not 10 or 11.

@phpfui
Copy link
Contributor

phpfui commented Jun 4, 2021

composer.json seems to be using psr-4 autoloading, and there is a src directory. So I think that is 10, as I put in a PR for that. Also see this:

    "extra": {
        "branch-alias": {
            "dev-master": "10.0-dev"
        }
    }

It could be 9, but what ever, it should be released. I don't really care, we need to get updated code out there and not be held hostage to quaint notions of needing backward compatibility back to the late Ming dynasty.

It works, ship it.

@joshgoebel
Copy link

It could be 9, but what ever, it should be released.

It is 9. And I agree with releasing - if the critical performance fix hasn't shipped, it should.

not be held hostage to quaint notions of needing backward compatibility back to the late Ming dynasty.

I have no idea what this is referring to. Older PHP versions or desire for backwards compatibility is NOT the reason there is no v10 or v11 release yet.

@allejo
Copy link
Collaborator Author

allejo commented Jun 4, 2021

Is there anything we can do to support the upgrade?

Any help needed on the code, or perhaps some sponsoring?

I'd definitely welcome a helping hand with starting to port core highlight.js changes to the PHP codebase 👍 If you have time to spare for contributions, let me know and I'll be glad to discuss the porting process.

Anyone sponsoring my time to work on this to catch up would be nice 😄 Regardless of sponsorships, I do intend on working on this in the coming weeks (hopefully) 🤞

I upgraded phpfui.com to dev-master and WOW! It is much faster. The largest file timed out before (170K) and everything seems super snappy now. I would recommend an immediate release of V10. It was plug in compatible. I did not have to change anything.

I wish I could tell you I worked hard at making things fast 😅 But as mentioned, the master branch (i.e. v10-dev) is pretty much the same as the 9.18 series with the exception of some reorganization of files.

As Josh mentioned, there was a significant speed fix but that exists in the 9.18 series. Maybe phpfui.com wasn't using the latest 9.18 series?

@allejo Did you make a release with that huge speed fix a while back? If not prolly should.

The speed fix you are referring to was released in v9.18.1.5. There is another unrelated speed fix in v9.18.1.6, but that is only specific to the splitCodeIntoArray() utility function.

@joshgoebel
Copy link

The speed fix you are referring to was released in v9.18.1.5. There is another unrelated speed fix in v9.18.1.6,

Well then I have no ideas here. :)

@phpfui
Copy link
Contributor

phpfui commented Jun 5, 2021

I did some more research on why I am seeing a performance improvement from v9.18.1.6 to master. The only thing that I see is this line:

preg_match_all($this->source, $str, $results, PREG_SET_ORDER | PREG_OFFSET_CAPTURE, $this->lastIndex);

PREG_SET_ORDER has been removed in master. You can see the change here: http://phpfui.com/?n=Highlight&c=RegEx&p=g#

I get a memory error on v9.18.1.6, but master runs quickly.

The other change I see is that master uses PSR-4 autoloading with a src directory, while v9.18.1.6 is using PRS-0. Not sure where this was introduced (I put in the PR a long time ago), but I was under the impression that PSR-4 would be in 10, which is why I was advocating for its release.

In any case, I think v9.18.1.7 would be in order based on master branch. Seems good to me.

@joshgoebel
Copy link

Could you make a quick edit to confirm which change is the cause of the performance difference?

@phpfui
Copy link
Contributor

phpfui commented Jun 5, 2021

Confirmed the last changes to Highlight\RegEx::exec method fixed a memory issue in previous version. Actually it is doing a bit more than just the one option, also changed from preg_match_all to just preg_match. I had to revert the entire method, as it was not a change to just remove the one option.

@allejo
Copy link
Collaborator Author

allejo commented Jun 5, 2021

I'm honestly not sure where you're finding the differences. In 9.18.1.4, there was the inefficient function call that PREG_SET_ORDER and preg_match_all. This was changed fixed in 9.18.1.6 to use preg_match and removed PREG_SET_ORDER. This change was then ported forward to the master branch.

master aka v10-dev

preg_match($this->source, $str, $results, PREG_OFFSET_CAPTURE, $this->lastIndex);

v9.18.1.6 tag

preg_match($this->source, $str, $results, PREG_OFFSET_CAPTURE, $this->lastIndex);

v9.18.1.4 tag

preg_match_all($this->source, $str, $results, PREG_SET_ORDER | PREG_OFFSET_CAPTURE, $this->lastIndex);

@phpfui
Copy link
Contributor

phpfui commented Jun 6, 2021

The only thing I can think of is a composer failure. I have seen that before where the wrong version gets installed due to cache issues. Master changed to PRS-4 and maybe that refreshed the cache?

@phpfui
Copy link
Contributor

phpfui commented Oct 27, 2022

Where are we with V10? I have been running dev-master in production for almost a year and it works great. No issues.

I think you can release master as V10 at this point.

@westonruter
Copy link

The current estimated release of highlight.php v10/11 is Winter 2021

I think this may be a bit out of date.

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

No branches or pull requests

5 participants