This change has been marked as submitted.

Describe the submission (optional):

Merged fixed version into master

Reviews
Diff

- Download Diff

Summary:

[General] Add support for installing dependencies through composer

Review Request #515 — Created April 24, 2017 and submitted 7 years, 10 months ago

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	8 years, 1 month ago
Do we really want the lock file in the repo?	pprkut	8 years, 1 month ago
this will fail if it's not present. Composer must stay optional	pprkut	8 years, 1 month ago
this is too new. We need 1.7	pprkut	8 years, 1 month ago
wrong repo	pprkut	8 years, 1 month ago
First, this needs to be the other way around. Second, even the other way around is still wrong, because also ...	pprkut	8 years ago
this is not how it works...	pprkut	8 years ago
"else" is fine	pprkut	8 years ago
these are from our own clones	smillernl	8 years ago
missing at least: - mysqlnd_ms - msgpack maybe also - gettext - mysqli	pprkut	8 years ago
where is this needed?	pprkut	8 years ago
Explanations here need to get to the point where people actually know what this is needed for. At the very ...	pprkut	8 years ago
I don't think that's the case anymore	pprkut	8 years ago
this can be anywhere, doesn't have to be in this directory.	smillernl	8 years ago
should be vendor/autoload.php	smillernl	8 years ago
why is this in here twice?	smillernl	8 years ago
this is default	smillernl	8 years ago
shouldn't this be src/Lunr/?	smillernl	8 years ago
RTFM / try again	smillernl	8 years ago
probably need to require a specific PHP version too, which would need to be 5.6 in our case.	pprkut	8 years ago
should non-pecl extension be listed here too?	pprkut	7 years, 12 months ago
I'd just reduce this to "Needed for mocking in unit tests"	pprkut	8 years ago
Missing the requirement for Shadow	pprkut	8 years ago
what are those version supposed to denote?	pprkut	8 years ago
"Needed for unit tests"	pprkut	8 years ago
"Extended" suggests there's "Basic" HTTP support without it, which is not true. Also, Shadow is not supporting HTTP at all. ...	pprkut	8 years ago
you can skip the libcapn part	pprkut	8 years ago
1.0.4 should be good enough	pprkut	8 years ago
not new enough	pprkut	8 years ago
I don't think this will work	pprkut	8 years ago
I think these should be less exact. It's unlikely that someone else will just include all of Lunr in their ...	pprkut	8 years ago
">=" is no good, since it doesn't shield against API breaks. There's a different operator for staying on the same ...	pprkut	8 years ago
xdebug doesn't need to be that strict. I think ~2.4 should be fine	pprkut	8 years ago
We need svn revision 335219, which is 2 years newer than 1.6.0	pprkut	7 years, 12 months ago
typically svn revisions are noted as r335219, is the format a given from composer's side?	pprkut	7 years, 12 months ago
you can also use UOPZ as per my latest review	smillernl	7 years, 11 months ago
this is non-functional 1) we have PHP 5.6 as required, but uopz 5 is PHP 7 only 2) it's either ...	pprkut	7 years, 10 months ago
Should be 5.7.*	smillernl	7 years, 10 months ago
There are no open issues

composer.json (Diff revision 1)

The issue has been resolved. Show all issues

Can you explain some more on what this all means?

p.valk 8 years, 1 month ago

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 8 years, 1 month ago

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

composer.lock (Diff revision 1)

The issue has been dropped. Show all issues

Do we really want the lock file in the repo?

p.valk 8 years, 1 month ago

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 8 years, 1 month ago
```
Fair enough
```

tests/test.bootstrap.inc.php (Diff revision 1)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
this is too new. We need 1.7
```
composer.lock (Diff revision 2)
The issue has been resolved. Show all issues
```
wrong repo
```

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

The issue has been resolved. 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 8 years, 1 month ago

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)

The issue has been resolved. Show all issues

this is not how it works...

p.valk 8 years ago

why not? stream_resolve_include_path returns the absolute path.

pprkut 8 years ago

stream_resolve_include_path() is file_exists()

tests/test.bootstrap.inc.php (Diff revisions 2 - 3)
The issue has been resolved. 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)

The issue has been resolved. Show all issues

missing at least:
- mysqlnd_ms
- msgpack

maybe also
- gettext
- mysqli

composer.json (Diff revision 5)

The issue has been resolved. Show all issues

where is this needed?

p.valk 8 years ago
```
i got this of the list from Sean.
```

p.valk 8 years ago

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

composer.json (Diff revision 5)
The issue has been resolved. Show all issues
```
these are from our own clones
```
1. p.valk 8 years ago
  add php-resque repo from m2mobi.

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

The issue has been resolved. Show all issues

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

p.valk 8 years ago

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

pprkut 8 years ago

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)
The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
why is this in here twice?
```
composer.json (Diff revision 7)
The issue has been resolved. Show all issues
```
this is default
```
composer.json (Diff revision 7)
The issue has been resolved. Show all issues
```
shouldn't this be src/Lunr/?
```
tests/test.bootstrap.inc.php (Diff revision 7)
The issue has been resolved. 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)

The issue has been resolved. Show all issues

should non-pecl extension be listed here too?

p.valk 8 years ago

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

pprkut 8 years ago

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

composer.json (Diff revision 9)
The issue has been dropped. Show all issues
```
I'd just reduce this to "Needed for mocking in unit tests"
```
1. p.valk 8 years ago
  NR in latest change.
composer.json (Diff revision 9)
The issue has been dropped. Show all issues
```
"Needed for unit tests"
```
1. p.valk 8 years ago
  NR in latest change.

composer.json (Diff revision 9)

The issue has been resolved. 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)
The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Missing the requirement for Shadow
```

composer.json (Diff revisions 9 - 10)

The issue has been resolved. Show all issues

what are those version supposed to denote?

p.valk 8 years ago
```
suggested versions
```

pprkut 8 years ago

ok, then pls fix the versions for gettext and mysqli

p.valk 8 years ago

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 8 years ago

Both of them are core extensions, not pecl.

composer.json (Diff revisions 9 - 10)
The issue has been resolved. Show all issues
```
1.0.4 should be good enough
```
composer.json (Diff revisions 9 - 10)
The issue has been resolved. Show all issues
```
not new enough
```
composer.lock (Diff revisions 9 - 10)
The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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 8 years ago

you mean the Next Significant Release Operators?

pprkut 8 years ago
```
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)
The issue has been resolved. Show all issues
```
xdebug doesn't need to be that strict. I think ~2.4 should be fine
```

composer.lock (Diff revisions 12 - 13)

The issue has been resolved. Show all issues

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

p.valk 8 years ago

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

pprkut 8 years ago
```
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)

The issue has been resolved. Show all issues

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

p.valk 7 years, 12 months ago

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

composer.json (Diff revision 14)
The issue has been resolved. Show all issues
```
you can also use UOPZ as per my latest review
```
1. p.valk 7 years, 11 months ago
  currently we are not using UOPZ yet right?
2. pprkut 7 years, 11 months ago
  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)

The issue has been resolved. 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 7 years, 11 months ago

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 7 years, 11 months ago

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

composer.lock (Diff revision 15)
The issue has been dropped. Show all issues
```
Should be 5.7.*
```
1. pprkut 7 years, 10 months ago
  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

This change has been marked as submitted.

[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: