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

[bug] invalid ast for const #210

Closed
alexander-akait opened this issue Oct 25, 2018 · 2 comments · Fixed by #227
Closed

[bug] invalid ast for const #210

alexander-akait opened this issue Oct 25, 2018 · 2 comments · Fixed by #227
Assignees

Comments

@alexander-akait
Copy link
Collaborator

Input:

const CONSTANT = 'Hello', TEST = 'world';

Output each const as separate item, but should have same ast as for static. I will look how we can fix it.

@ichiriac
Copy link
Member

Here some hints because you have multiple parts to change :

You can start here :

return this.next().read_const_list();

You may have a location problem with this node

You have to change the way this function works by changing the node instanciation :

read_const_list: function() {

You may have to change the signature of the AST node :
https://github.com/glayzzle/php-parser/blob/master/src/ast/constant.js

  1. You can use the expression statement with this node as it expects the endOfStatement :
    this.expectEndOfStatement();

Note : every expression waiting an endOfStatement should be wrapped on an expressionstatement node.

  1. You may need to create a new ConstantDeclaration node extending the Expression Node, having a list of constant nodes.
    After declaring the new node into its file you have to register it here :

  2. Use the new node into the read_const_list function

@ichiriac
Copy link
Member

After you will have to also migrate const keyword from classes :
https://github.com/glayzzle/php-parser/blob/master/src/parser/class.js#L80

In order to do this, you have to create a ClassConstantDeclaration (that replaces the ClassConstant usage) extending the ConstantDeclaration and introducing the visibility flag.

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

Successfully merging a pull request may close this issue.

2 participants