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

[Core] Index fields name configuration duplication #200

Open
alexander-schranz opened this issue Jun 13, 2023 Discussed in #199 · 1 comment
Open

[Core] Index fields name configuration duplication #200

alexander-schranz opened this issue Jun 13, 2023 Discussed in #199 · 1 comment
Labels
DX Improves the developer experience features New feature or request SEAL Core Seal Core related issue

Comments

@alexander-schranz
Copy link
Member

alexander-schranz commented Jun 13, 2023

Discussed in #199 @kbond

Originally posted by kbond June 13, 2023
This is a very tiny papercut I had - not a big deal.

Consider this:

use Schranz\Search\SEAL\Schema\Field;
use Schranz\Search\SEAL\Schema\Index;

return new Index('page', [
    'id' => new Field\IdentifierField('id'),
    'title' => new Field\TextField('title'),
    'subtitle1' => new Field\TextField('subtitle1'),
    'subtitle2' => new Field\TextField('subtitle2'),
    'description' => new Field\TextField('description'),
]);

I think it would be nice if we didn't have to set the array key:

use Schranz\Search\SEAL\Schema\Field;
use Schranz\Search\SEAL\Schema\Index;

return new Index('page', [
    new Field\IdentifierField('id'),
    new Field\TextField('title'),
    new Field\TextField('subtitle1'),
    new Field\TextField('subtitle2'),
    new Field\TextField('description'),
]);

I could be missing something but if interested, feel free to turn into an issue and I can create a PR.

Originally posted by alexander-schranz June 13, 2023
This is currently also related if we use the same classes maybe in a future Object Data Mapper package or not. As it could also be:

use Schranz\Search\SEAL\Schema\Field;
use Schranz\Search\SEAL\Schema\Index;

return new Index('page', [
    'id' => new Field\IdentifierField(),
    'title' => new Field\TextField(),
    'subtitle1' => new Field\TextField(),
    'subtitle2' => new Field\TextField(),
    'description' => new Field\TextField(),
]);

The key is currently required inside Index.php class as the items are currently accessed that way.

In a ODM the above example could maybe look like this:

#[Index('page')]
class Test {
    #[Field\IdentifierField()]
    public string $id;
}

What I'm not yet sure if I should support something like doctrine does fieldName vs columnName which would be something like this:

return new Index('page', [
    'fieldName' => new Field\TextField('columnName'),
]);

At current state that is not supported and currently avoding that to avoid to have complexity in the mapping of the stored data vs presentated data, but the initial interface was created by keep that in mind and that is way it currently looks like this.

I think it should not hurt the performance too much if we currently add support for int as field key but make sure that $index->fields always use the name as that is required to access the field. So we could adopt the __construct of Index do something like this:

$normalizedFields = [];
foreach ($fields as $key => $field) {
    if (is_int($key)) {
         $key = $field->name;
    }
    
    $this->normalizedFields[$key] = $field;
}

$this->fields = $normalizedFields; // set to the readonly publi

This needs also keep in mind for TypedField and ObjectField which also have fields configuration. If you want to give it a try feel free to create a pull request for it.

This issue is related to #107

@alexander-schranz alexander-schranz added features New feature or request SEAL Core Seal Core related issue DX Improves the developer experience labels Jun 13, 2023
@alexander-schranz alexander-schranz changed the title Index fields configuration duplication Index fields name configuration duplication Oct 6, 2024
@alexander-schranz
Copy link
Member Author

To avoid errors at current state we will add validation in #427. To avoid errors for false keys at current state. Still in future a mapping may be possible.

@alexander-schranz alexander-schranz changed the title Index fields name configuration duplication [Core] Index fields name configuration duplication Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Improves the developer experience features New feature or request SEAL Core Seal Core related issue
Projects
None yet
Development

No branches or pull requests

1 participant