-
Notifications
You must be signed in to change notification settings - Fork 22
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
bump to php7.4 #24
base: master
Are you sure you want to change the base?
bump to php7.4 #24
Conversation
Hi, James, any chance you can take a look at this? In particular, I don't know why the search test fails, I removed the 2 tests, but obviously that's not the right approach. Thx. |
@@ -8,6 +8,7 @@ | |||
|
|||
namespace VertigoLabs\DoctrineFullTextPostgres\Common; | |||
|
|||
use App\Entity\Media; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shouldn't exist
// $this->checkWatchFields($class, $prop, $attribute); | ||
// $metaData->mapField([ | ||
// 'fieldName' => $prop->getName(), | ||
// 'columnName' => $this->getColumnName($prop, $annotation), | ||
// 'type' => 'tsvector', | ||
// 'weight' => strtoupper($annotation->weight), | ||
// 'language' => strtolower($annotation->language), | ||
// 'nullable' => $this->isWatchFieldNullable($class, $annotation) | ||
// ]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code should be removed
foreach ($prop->getAttributes() as $reflectionAttribute) { | ||
if ($reflectionAttribute->getName() === TsVector::class) { | ||
// /** @var TsVector $attribute */ | ||
$attribute = new ($reflectionAttribute->getName())(...$reflectionAttribute->getArguments()); // crazy, something is wrong here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little apprehensive about this comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No doubt! I'll see if I can figure it out. I'm fighting a little bit with supporting both attributes and annotations, I think that attributes should take priority if both exist.
tests aren't working yet.
The current 'master' branch has a minimum stability of 'dev'. I think before this PR is accepted, perhaps you could drop that line, make sure everything runs, then tag is as 1.0, so anyone currently using it has a safe way of keeping it.
Alternatively, maybe create another branch and merge these changes in?
I'm not actually that familiar with how to use tsvector and such, which is why I used this bundle. So if you can fix the tests, I can clean up the code deprecations and such.
Thanks.