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

bump to php7.4 #24

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

bump to php7.4 #24

wants to merge 10 commits into from

Conversation

tacman
Copy link
Contributor

@tacman tacman commented Feb 10, 2022

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.

@tacman
Copy link
Contributor Author

tacman commented Feb 12, 2022

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;
Copy link
Owner

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

Comment on lines +101 to +110
// $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)
// ]);

Copy link
Owner

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.
Copy link
Owner

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!

Copy link
Contributor Author

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.

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.

2 participants