- Download Diff

Summary:

[General] Add support for installing dependencies through composer

Review Request #515 — Created April 24, 2017 and submitted Aug. 2, 2017, 10:57 a.m.

Information
Owner:	p.valk
Repository:	Lunr
Branch:	composer
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Added Composer.json

Testing Done

manual

Issues

0
0
34
4
All Issues: 38

Description	From	Last Updated
Can you explain some more on what this all means?	pprkut	April 24, 2017, 4:47 p.m.
Do we really want the lock file in the repo?	pprkut	April 24, 2017, 4:32 p.m.
this will fail if it's not present. Composer must stay optional	pprkut	April 24, 2017, 3:38 p.m.
this is too new. We need 1.7	pprkut	April 24, 2017, 4:47 p.m.
wrong repo	pprkut	April 24, 2017, 4:47 p.m.
First, this needs to be the other way around. Second, even the other way around is still wrong, because also ...	pprkut	May 3, 2017, 2:37 p.m.
this is not how it works...	pprkut	May 10, 2017, 9:48 a.m.
"else" is fine	pprkut	May 10, 2017, 9:48 a.m.
these are from our own clones	smillernl	May 10, 2017, 3:12 p.m.
missing at least: - mysqlnd_ms - msgpack maybe also - gettext - mysqli	pprkut	May 10, 2017, 3:17 p.m.
where is this needed?	pprkut	May 10, 2017, 3:30 p.m.
Explanations here need to get to the point where people actually know what this is needed for. At the very ...	pprkut	May 17, 2017, 10 a.m.
I don't think that's the case anymore	pprkut	May 10, 2017, 5:08 p.m.
this can be anywhere, doesn't have to be in this directory.	smillernl	May 10, 2017, 3:33 p.m.
should be vendor/autoload.php	smillernl	May 10, 2017, 3:12 p.m.
why is this in here twice?	smillernl	May 10, 2017, 5 p.m.
this is default	smillernl	May 10, 2017, 5 p.m.
shouldn't this be src/Lunr/?	smillernl	May 10, 2017, 5:14 p.m.
RTFM / try again	smillernl	May 10, 2017, 5 p.m.
probably need to require a specific PHP version too, which would need to be 5.6 in our case.	pprkut	May 17, 2017, 1:16 p.m.
should non-pecl extension be listed here too?	pprkut	May 31, 2017, 4:02 p.m.
I'd just reduce this to "Needed for mocking in unit tests"	pprkut	May 17, 2017, 1:06 p.m.
Missing the requirement for Shadow	pprkut	May 17, 2017, 1:19 p.m.
what are those version supposed to denote?	pprkut	May 17, 2017, 3:14 p.m.
"Needed for unit tests"	pprkut	May 17, 2017, 1:06 p.m.
"Extended" suggests there's "Basic" HTTP support without it, which is not true. Also, Shadow is not supporting HTTP at all. ...	pprkut	May 17, 2017, 1:06 p.m.
you can skip the libcapn part	pprkut	May 17, 2017, 1:06 p.m.
1.0.4 should be good enough	pprkut	May 17, 2017, 1:20 p.m.
not new enough	pprkut	May 17, 2017, 1:20 p.m.
I don't think this will work	pprkut	May 17, 2017, 1:31 p.m.
I think these should be less exact. It's unlikely that someone else will just include all of Lunr in their ...	pprkut	May 26, 2017, 3:26 p.m.
">=" is no good, since it doesn't shield against API breaks. There's a different operator for staying on the same ...	pprkut	May 26, 2017, 3:26 p.m.
xdebug doesn't need to be that strict. I think ~2.4 should be fine	pprkut	May 26, 2017, 4:04 p.m.
We need svn revision 335219, which is 2 years newer than 1.6.0	pprkut	May 31, 2017, 3:24 p.m.
typically svn revisions are noted as r335219, is the format a given from composer's side?	pprkut	May 31, 2017, 4:52 p.m.
you can also use UOPZ as per my latest review	smillernl	June 7, 2017, 9:45 a.m.
this is non-functional 1) we have PHP 5.6 as required, but uopz 5 is PHP 7 only 2) it's either ...	pprkut	Aug. 2, 2017, 10:55 a.m.
Should be 5.7.*	smillernl	Aug. 2, 2017, 10:54 a.m.

composer.json (Diff revision 1)

Show all issues

Can you explain some more on what this all means?

p.valk April 24, 2017, 3:31 p.m.

repositories points to custom repositories to search for the versions we want.
require is a list of what librarys we need and which version of those libraries.
of some libraries we want a specific git commit, we need to do this in a specific style dev-'branch name'#'commit hash'
the config "optimize-autoloader": true makes composer optimise its output, the install process is a bit slower but they recommend to use this on live to make to compuser loader process faster.

pprkut April 24, 2017, 4:32 p.m.

Then I don't think we need to point to repos that are already the default, no?

composer.lock (Diff revision 1)

Show all issues

Do we really want the lock file in the repo?

p.valk April 24, 2017, 3:26 p.m.

well they advice to put it in the repo, this way everyone will have the same version of the libraries. (when doing compose install)
when you update composer update the lock file changes and this would be reflected in the reviews/git.

i agree this seems usefull.

pprkut April 24, 2017, 4:32 p.m.
```
Fair enough
```

tests/test.bootstrap.inc.php (Diff revision 1)

Show all issues

this will fail if it's not present. Composer must stay optional

Commit:

-	48157012d53f76bc2567d9b46d5dbe6178c61c9d
+	05a1f6d5fdc790f9bc9d9826e51d2494d5eb19f3

Diff:

Revision 2 (+340 -1)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.lock (Diff revision 2)
Show all issues
```
this is too new. We need 1.7
```
composer.lock (Diff revision 2)
Show all issues
```
wrong repo
```

tests/test.bootstrap.inc.php (Diff revision 2)

Show all issues

First, this needs to be the other way around.
Second, even the other way around is still wrong, because also include_once barks if the file does not exist.

p.valk April 24, 2017, 4:42 p.m.

ah my mistake, replaced the wrong one.
include only barks, whats wrong with that? it doesnt crash~

Commit:

-	05a1f6d5fdc790f9bc9d9826e51d2494d5eb19f3
+	f532d8a8841ef4144037a866154480fc4b5532f4

Diff:

Revision 3 (+352 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

tests/test.bootstrap.inc.php (Diff revisions 2 - 3)

Show all issues

this is not how it works...

p.valk May 3, 2017, 6:01 p.m.

why not? stream_resolve_include_path returns the absolute path.

pprkut May 3, 2017, 6:04 p.m.

stream_resolve_include_path() is file_exists()

tests/test.bootstrap.inc.php (Diff revisions 2 - 3)
Show all issues
```
"else" is fine
```

Commit:

-	f532d8a8841ef4144037a866154480fc4b5532f4
+	75246ef33bf0a881086579b3ff2820657ba156be

Diff:

Revision 4 (+352 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

Looks better now, but the pecl extensions are missing :)

Change Summary:

please check if the suggest messages are ok.

Commit:

-	75246ef33bf0a881086579b3ff2820657ba156be
+	110c897e57ee2dd205f8ab0652cdba8ae501c800

Diff:

Revision 5 (+359 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revision 5)

Show all issues

missing at least:
- mysqlnd_ms
- msgpack

maybe also
- gettext
- mysqli

composer.json (Diff revision 5)

Show all issues

where is this needed?

p.valk May 10, 2017, 3:06 p.m.
```
i got this of the list from Sean.
```

p.valk May 10, 2017, 3:27 p.m.

i cant find it in the lunr project, i'l remove it from the list.

composer.json (Diff revision 5)
Show all issues
```
these are from our own clones
```
1. p.valk May 10, 2017, 3:08 p.m.
  add php-resque repo from m2mobi.

tests/test.bootstrap.inc.php (Diff revision 5)

Show all issues

this can be anywhere, doesn't have to be in this directory.

p.valk May 10, 2017, 3:33 p.m.

set a config key so the vendor directory is always at the same place.

pprkut May 10, 2017, 4:46 p.m.

That was not the point. The point was that stream_resolve_include_path() could pick up any vendor/vendor.php, not specifically the one from Lunr.

tests/test.bootstrap.inc.php (Diff revision 5)
Show all issues
```
should be vendor/autoload.php
```

Commit:

-	110c897e57ee2dd205f8ab0652cdba8ae501c800
+	1caa42811702b3a56c1a914198c5c15e11f96cc3

Diff:

Revision 6 (+367 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revisions 5 - 6)

Show all issues

Explanations here need to get to the point where people actually know what this is needed for. At the very least this needs to mention the packages they are used in, and even better would be if they mentioned for what.

composer.lock (Diff revisions 5 - 6)
Show all issues
```
I don't think that's the case anymore
```

Commit:

-	1caa42811702b3a56c1a914198c5c15e11f96cc3
+	5d38613db96b55bf39867bf76be5b3096a481739

Diff:

Revision 7 (+367 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revision 7)
Show all issues
```
why is this in here twice?
```
composer.json (Diff revision 7)
Show all issues
```
this is default
```
composer.json (Diff revision 7)
Show all issues
```
shouldn't this be src/Lunr/?
```
tests/test.bootstrap.inc.php (Diff revision 7)
Show all issues
```
RTFM / try again
```

Change Summary:

i need to check the other suggest messages more but at least you can review this for when i am back next week.

Commit:

-	5d38613db96b55bf39867bf76be5b3096a481739
+	ce7c21be651cd006a04f0c3f313cb8b2b2149312

Diff:

Revision 8 (+361 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

Commit:

-	ce7c21be651cd006a04f0c3f313cb8b2b2149312
+	de5c40e1f27313fc074dd9ed90e3fe4f845ebab1

Diff:

Revision 9 (+361 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revision 9)

Show all issues

should non-pecl extension be listed here too?

p.valk May 17, 2017, 1:08 p.m.

possible, suggest will only give a notification so you can add anything you want.

pprkut May 17, 2017, 4:36 p.m.

You can grep for "requires extension" and add the missing ones

composer.json (Diff revision 9)

Show all issues

I'd just reduce this to "Needed for mocking in unit tests"

p.valk May 17, 2017, 1:06 p.m.
```
NR in latest change.
```

composer.json (Diff revision 9)
Show all issues
```
"Needed for unit tests"
```
1. p.valk May 17, 2017, 1:06 p.m.
  NR in latest change.

composer.json (Diff revision 9)

Show all issues

"Extended" suggests there's "Basic" HTTP support without it, which is not true. Also, Shadow is not supporting HTTP at all. It's using pecl_http for other reasons

composer.json (Diff revision 9)
Show all issues
```
you can skip the libcapn part
```

Commit:

-	de5c40e1f27313fc074dd9ed90e3fe4f845ebab1
+	22dbed54edb0e233dd63beeb3e03981cd964458c

Diff:

Revision 10 (+370 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revisions 9 - 10)

Show all issues

probably need to require a specific PHP version too, which would need to be 5.6 in our case.

composer.json (Diff revisions 9 - 10)
Show all issues
```
Missing the requirement for Shadow
```

composer.json (Diff revisions 9 - 10)

Show all issues

what are those version supposed to denote?

p.valk May 17, 2017, 1:19 p.m.
```
suggested versions
```

pprkut May 17, 2017, 1:23 p.m.

ok, then pls fix the versions for gettext and mysqli

p.valk May 17, 2017, 1:40 p.m.

well those are versions i got reported by composer xD
not sure which versions we need/use?
both of them the pecl pages are dead too.

pprkut May 17, 2017, 1:52 p.m.

Both of them are core extensions, not pecl.

composer.json (Diff revisions 9 - 10)
Show all issues
```
1.0.4 should be good enough
```
composer.json (Diff revisions 9 - 10)
Show all issues
```
not new enough
```
composer.lock (Diff revisions 9 - 10)
Show all issues
```
I don't think this will work
```

Commit:

-	22dbed54edb0e233dd63beeb3e03981cd964458c
+	95c21264cc66a71422012552227a75872d91ba33

Diff:

Revision 11 (+371 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revisions 10 - 11)

Show all issues

I think these should be less exact. It's unlikely that someone else will just include all of Lunr in their project (particularly because I won't put it on packagist), but if someone would do it, the exact versions would cause conflicts.
Just stick to the respective stable branches, except for the ones that need specific versions (mysqldn_ms, php-resque, apns-php)

Commit:

-	95c21264cc66a71422012552227a75872d91ba33
+	a710a2bd89a7b7d35c58e9e6769f2bce7d58465c

Diff:

Revision 12 (+371 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revisions 11 - 12)

Show all issues

">=" is no good, since it doesn't shield against API breaks. There's a different operator for staying on the same stable branch

p.valk May 26, 2017, 3:14 p.m.

you mean the Next Significant Release Operators?

pprkut May 26, 2017, 3:17 p.m.
```
yes
```

Commit:

-	a710a2bd89a7b7d35c58e9e6769f2bce7d58465c
+	6f7484bee989d4c1af3405f5ceddc35cdc372069

Diff:

Revision 13 (+371 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revisions 12 - 13)

Show all issues

xdebug doesn't need to be that strict. I think ~2.4 should be fine

composer.lock (Diff revisions 12 - 13)

Show all issues

We need svn revision 335219, which is 2 years newer than 1.6.0

p.valk May 26, 2017, 4:09 p.m.

this one? http://svn.php.net/viewvc?view=revision&revision=335219

pprkut May 26, 2017, 4:14 p.m.
```
yes
```

Commit:

-	6f7484bee989d4c1af3405f5ceddc35cdc372069
+	b8bea5ce2c20aac0dce2ac9eca05a0af4426da3b

Diff:

Revision 14 (+370 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

composer.json (Diff revisions 13 - 14)

Show all issues

typically svn revisions are noted as r335219, is the format a given from composer's side?

p.valk May 31, 2017, 4:52 p.m.

i didnt know that. but its just a suggestion. composer will never install it automatically from this point.

composer.json (Diff revision 14)

Show all issues

you can also use UOPZ as per my latest review

p.valk June 7, 2017, 9:06 a.m.

currently we are not using UOPZ yet right?

pprkut June 7, 2017, 9:16 a.m.
```
We are, but only for PHP7
```

Commit:

-	b8bea5ce2c20aac0dce2ac9eca05a0af4426da3b
+	64cf0b1b9b7a85f6fa5adf1eea3252d3b75af1c8

Diff:

Revision 15 (+372 -4)

Show changes

	.gitignore
	composer.lock
	composer.json
	tests/test.bootstrap.inc.php

Summary:

-	Added Composer.json
+	[Added] Composer.json

Change Summary:

Fix summary

Summary:

-	[Added] Composer.json
+	[General] Add support for installing dependencies through composer

composer.json (Diff revision 15)

Show all issues

this is non-functional

1) we have PHP 5.6 as required, but uopz 5 is PHP 7 only
2) it's either runkit or uopz, not both. They don't even run on the same PHP version

p.valk June 7, 2017, 4:59 p.m.

so either we move them both to suggested or make the composer file only for php 5.6; this as it already recuires php version 5.6.*.

pprkut June 7, 2017, 5:30 p.m.

I'd just move uopz to suggests and note when it is needed.

composer.lock (Diff revision 15)

Show all issues

Should be 5.7.*

pprkut July 28, 2017, 10:38 a.m.

That's the phpunit requirement of php-resque, not ours

Status: Discarded

Status: Closed (submitted)

Change Summary:

Merged fixed version into master

You have a pending review.

Review Board 3.0.24

[General] Add support for installing dependencies through composer

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Summary:

Change Summary:

Summary:

Status: Discarded

Status: Closed (submitted)

Change Summary: