search

libgraviton / php-rql-parser

A PHP RQL Parsing Library
15 stars 4 forks source link

Rewrite Parser approach #6

Closed izziaraffaele closed 9 years ago

izziaraffaele commented 9 years ago

I was trying to implement them by my self but I could not get the regex working. I'm not so good at regex :D

What I was trying to do is parsing a query like this "(eq(foo,3)&lt(price,10))|eq(bar,5)" and I was expecting to get it parsed according to what you written in the README but actually it is not.

Could you please help me parsing this query so then I can fix my Queriable class to handle it?

Thanks a lot

narcoticfresh commented 9 years ago

Hi Raffaele

I just verified that this query doesn't get parsed properly..

See, the current Parser implementation tries to follow a 'one regex to rule them all' approach. I think this approach is flawed and we should not go on like this. The regex there is already now hard to read and as we see - it doesn't really work in all cases.

I think we need to re-write this Parser part. As I can see it, we should implement a Lexer based approach. Sadly, we don't have access to links like ANTLR[1] in PHP - so a simple Lexing thing would be best.

As a reference, a small Lexing class could do the job. As a reference we could look how Twig[2] does it and implement something like it. The job of parsing a template is kinda comparable of parsing our expression.

I hope either me or @hairmare get some time from our company to rewrite that ;-) Or maybe you have some resources?

[1] http://www.antlr.org/ [2] http://twig.sensiolabs.org/doc/internals.html

narcoticfresh commented 9 years ago

Hm, this looks interesting: https://github.com/jakubledl/dissect/

izziaraffaele commented 9 years ago

Let's see if I can help in some ways.

BTW we can also take inspiration from a SQL parser like this https://code.google.com/p/php-sql-parser. It makes use of the technique you specified before and can handle also complex cases

hairmare commented 9 years ago

@narcoticfresh From a quick glance it looks like we can use dissect to parse our input into an AST, we could then replace the Queriable with a node visitor that iterates over the AST and applies queries. We might need to do some kind of normalization on the input before parsing. I have to check out dissect more to tell. I also need to get my work on fixtures and acceptance testing done before we can even think about implementing this

hairmare commented 9 years ago

jakubledl/dissect seems to lack a maintainer. We might want to look into doctrine/lexer.

hairmare commented 9 years ago

I've got some initial work on a doctrine/lexer based parser done in the feature/lexer-parser-approach branch.

Right now my branch almost supports something very similar to the string in this issue, namely this or(and(eq(foo,3),lt(price,10)),eq(bar,5)).

I plan on adding syntax sugar like &, | and () at a later stage as I haven't figured out if I'd rather canonicalise input or extend the existing parser.

I'll open up a PR as soon as #7 has been merged. My change are rather extensive so they might take a while to get merged. I'm planning on doing a 2.0.0-alpha1 release shortly after.

hairmare commented 9 years ago

The last example in test/ParserTest.php on #11 shows nested queries working as per my last comment.

I'll try to get it merged next week to close this issue. The README has some docs on writing your own visitor.

@izziaraffaele Could you check if the visitor interface and AST will work for your use case?

izziaraffaele commented 9 years ago

@hairmare I just give a fast check and looks like it will. I will try to implement my visitor for Orchestrate on monday and I'll give you more detailed feedbacks. Thanks for your work guys!

izziaraffaele commented 9 years ago

@hairmare I just tried implementing my visitor for Orchestrate, you can have a look at it here https://github.com/izziaraffaele/php-rql-parser/tree/ast-refactor-orchestrate. The problem is that running the test suite I made I get "LogicException: missing close parenthesis". Is it possible that there are problems parsing query with "-"?

hairmare commented 9 years ago

@izziaraffaele + and - are fixed in 6e642aa. Thanks for noticing, I had them in the back of my head but missed implementing them.

izziaraffaele commented 9 years ago

@hairmare Thanks a lot. it is the solution I was thinking about but I didn't had idea of where to put the code ;)

hairmare commented 9 years ago

The ast-refactor has been landed on dev so I'm closing this. Feel free to create a new issue if you need anything.

I'll also create some issues based on the notes in the now merged #11. Maybe some of them are things you would like to tackle?

hairmare commented 9 years ago

Also: Thanks for testing and feedback.