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

Epic refactor for "Field" class #314

Merged
merged 24 commits into from
Feb 4, 2019
Merged

Conversation

romaninsh
Copy link
Member

@romaninsh romaninsh commented Mar 23, 2018

Currently "Field" class is to heavy. I reckon it can be refactored without BC breaks and help with with the clean-ups originally intended for later - #306.

This Epic PR will implement #308 through individual smaller PRs:

  • Cleanly get rid of Feld_SQL and add field factory.

The list may change as we go forward. Each separate PR will be discussed.

  • Implement Field\Boolean Implement Field\Boolean #315
  • Implement Field\Number Add support for more fields #375
  • Implement Field\List
  • Implement Field\Date
  • Implement Field\UUID
  • Implement Field\Location
  • Implement Field\Geometry
  • Implement Field\FullText
  • Implement containment (kinda containsMany / containsOne)
  • Implement Field\Measurement

Additionally for consideration:

  • Mechanism for field to verify persistence requirement and use different implementations
  • Take on handling of addCondition() $m->addCondition('home_location', '<10km', $xy)
  • Support individual features of fields, e.g. $m->intersects('area_field', $xy);

@codecov
Copy link

codecov bot commented Mar 23, 2018

Codecov Report

Merging #314 into develop will increase coverage by 0.13%.
The diff coverage is 84.9%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop    #314      +/-   ##
============================================
+ Coverage      85.06%   85.2%   +0.13%     
- Complexity      1019    1028       +9     
============================================
  Files             20      21       +1     
  Lines           2243    2264      +21     
============================================
+ Hits            1908    1929      +21     
  Misses           335     335
Impacted Files Coverage Δ Complexity Δ
src/Field_SQL.php 100% <ø> (ø) 4 <0> (-8) ⬇️
src/Model.php 91.57% <100%> (+0.07%) 325 <0> (+4) ⬆️
src/Field.php 72.58% <42.85%> (-3.76%) 75 <3> (-8)
src/Field/Boolean.php 85% <85%> (ø) 12 <12> (?)
src/Persistence_SQL.php 75.46% <94.44%> (+0.82%) 181 <8> (+9) ⬆️
src/Field/Callback.php 100% <0%> (+100%) 3% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e3d900...157f574. Read the comment docs.

@FrancessFractal
Copy link

Something you might add to 'for consideration' is data types made up of multiple fields. For instance, Postgres' point datatype might need two columns to be represented in MySQL. Alternatively, you could say that point needs to be broken into two fields...

@romaninsh
Copy link
Member Author

This is doable on a model level:

  1. declare super-field
  2. mark it non perist
  3. create 2-3 shadow fields.
  4. mark them system (hide)
  5. beforeSave copy value into shadow fields.
  6. afterLoad copy value back.

All that can be also defined inside a field class logic. I don't want to use it by default, however. Perhaps some for "Address/Location" field to store geo coordinates.

@romaninsh
Copy link
Member Author

romaninsh commented Apr 2, 2018

Another thing to add for consideration is if fields should care about their "visibility" for the UI. Currently there is no good way for ATK UI to implement this, but I don't see how this is even relevant in "ATK Data" domain:

https://github.com/atk4/data/blob/develop/src/Field.php#L418

https://forum.agiletoolkit.org/t/visible-hidden-editable-flags-do-nothing/465

Also - consider read-only fields.

@DarkSide666
Copy link
Member

I think it's data domain where we can set if field is visible, editable, hidden, system. UI should take these switches into account not only in CRUD component, but all components, including single form fields too.

Of course you can always overwrite that logic in form/field, but model is perfect place for default definition of visibility/editability.

@romaninsh
Copy link
Member Author

As @DarkSide666 pointed out, we also need "Blob" type.

@romaninsh romaninsh merged commit b8f75b0 into develop Feb 4, 2019
@romaninsh romaninsh deleted the feature/epic-field-refactor branch November 24, 2019 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants