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

Add patchers #67

Merged
merged 8 commits into from
Jul 6, 2017
Merged

Add patchers #67

merged 8 commits into from
Jul 6, 2017

Conversation

theofidry
Copy link
Member

@theofidry theofidry commented Jun 29, 2017

Attempt to replace #65. It's still not finished as:

  • It doesn't really work yet (the e2e tests still fails)
  • Tests are missing
  • Maybe something else I forgot

Closes #63

@padraic if you're happy with the idea I'll try to finish that PR (unless you want to finish it ofc).

@padraic
Copy link
Collaborator

padraic commented Jul 3, 2017

@theofidry Fire away and finish! :)

@theofidry
Copy link
Member Author

@padraic were the patches that you applied enough? The e2e tests are still failing for me so maybe I got them wrong...

@padraic
Copy link
Collaborator

padraic commented Jul 4, 2017

@theofidry Hmm, it just needed those two patches. It could just be the makefile - if you check my PR, I made some changes in there to get everything passing: https://github.com/humbug/php-scoper/pull/65/files#diff-b67911656ef5d18c4ae36cb6741b7965L64

@theofidry
Copy link
Member Author

Will take a look

@theofidry theofidry changed the title PoC: Add patchers Add patchers Jul 4, 2017
@theofidry
Copy link
Member Author

theofidry commented Jul 4, 2017

Ok added some tests. I'm still a bit unhappy with the name as it's the same as the binary one which can be a bit confusing...

Also regarding the error I could not find the source, however it looks like the regex doesn't anything (the code we are looking for doesn't exists anymore). I suppose this may be due to having different versions of PHP-Parser... Given how tightly coupled to it we are, we should fix it to a patch version and/or publish the composer.lock version.

I think publish the .lock version makes sense: it's mandatory if we want to publish php-scoper as a PHAR, i.e. treat it as an application. And when required with Composer, no patch is needed to make it work.

I think however that this should be fixed in another PR.

@theofidry theofidry requested a review from padraic July 5, 2017 10:18
@padraic
Copy link
Collaborator

padraic commented Jul 5, 2017

PHP-Parser 3.0.6 was released about a week back. Checking it, I think they updated one line we are seeking to replace (removing backslash escapes and now using single backslashes): https://github.com/nikic/PHP-Parser/blob/0808939f81c1347a3c8a82a5925385a08074b0f1/lib/PhpParser/Lexer.php#L363

Updating the regular expression used ought to fix that as an issue.

@theofidry
Copy link
Member Author

@Nyholm any suggestions for the config file name?

@Nyholm
Copy link
Contributor

Nyholm commented Jul 5, 2017

Lets keep it simple: scoper.ext or php-scoper.ext.

@theofidry
Copy link
Member Author

.ext? I would rather keep the .php, but scoper.php sounds better 👍

@padraic
Copy link
Collaborator

padraic commented Jul 6, 2017

We could always return to the convention for include files: scoper.inc

@theofidry theofidry force-pushed the feature/string-replace branch from fff1d6f to e5661ac Compare July 6, 2017 18:30
@theofidry theofidry merged commit 449a592 into humbug:master Jul 6, 2017
@theofidry theofidry deleted the feature/string-replace branch July 6, 2017 18:34
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.

3 participants