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

Exceptions if slug field not exists or its length is NULL #44

Open
wants to merge 6 commits into
base: 1.next
Choose a base branch
from
Open

Exceptions if slug field not exists or its length is NULL #44

wants to merge 6 commits into from

Conversation

burzum
Copy link
Contributor

@burzum burzum commented Mar 2, 2020

The exception if the DB field is missing is mostly a "in your face" notice for the developer.

The slug field should always have a fixed length. If it doesn't there might be problems with different DB systems / configs using different default lengths.

The exception if the DB field is missing is mostly a "in your face" notice for the developer.

The slug field should always have a fixed length. If it doesn't there might be problems with different DB systems / configs using different default lengths.
@jadb jadb requested a review from ADmad March 2, 2020 16:10
@jadb
Copy link
Member

jadb commented Mar 2, 2020

The changes look like an improvement to me.

Haven't played with TravisCI in a while. @ADmad maybe you can quickly spot why some tests are failing? I see some CS related stuff but other are for Pgsql?

@ADmad
Copy link
Member

ADmad commented Mar 2, 2020

I wonder if there's a use case where someone wants to set the slug to an entity property which doesn't map to a db field.

@jadb Travis now requires mysql and postgres to be explicitly enabled using services config.

@burzum
Copy link
Contributor Author

burzum commented Mar 3, 2020

@ADmad there are plenty of ways this can happen.

  • DB was updated but fixture not updated by someone (happend to us)
  • Migrations were not applied (forgotten by dev)
  • SQL file or DB was not manually updated (if people dont use migrations)
  • DB field was accidentally deleted
  • Typo in the DB field somewhere (table itself, behavior config)

I prefer robust code and exception messages that slap the cause in my face over saving propbably less than a microsecond CPU time by not executing the additional check.

@ADmad
Copy link
Member

ADmad commented Mar 3, 2020

@burzum I am not denying the usefulness of these explicit checks. My concern is adding check for table columns means one can't use the behavior to assign the slug to a virtual property, so I was wondering if there might be someone out there with such a use case.

@jadb
Copy link
Member

jadb commented Mar 4, 2020

I agree with @ADmad. While I don't have such a use case in mind, I do have a suggestion for a solution.

Adding isVirtual config would allow us to do an early check for that before looking/validating for slug column and/or its length. By default, the $this->getConfig('isVirtual') will be falsy unless explicitly defined which would skip the checks. Looking at the code, we'd also have to move up the lines normalizing the unique config so the isVirtual check can return early when false.

What do you guys think?

@ADmad
Copy link
Member

ADmad commented Mar 4, 2020

@jadb Sounds good.

@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #44 into 1.next will decrease coverage by 2.45%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             1.next      #44      +/-   ##
============================================
- Coverage     98.58%   96.12%   -2.46%     
- Complexity       59       62       +3     
============================================
  Files             3        3              
  Lines           141      155      +14     
============================================
+ Hits            139      149      +10     
- Misses            2        6       +4     

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 e36fe29...3a78be1. Read the comment docs.

@ADmad ADmad changed the base branch from master to 1.next March 6, 2020 19:17
jadb added 2 commits March 6, 2020 17:56
MySQL seems to default the length even when not defined, Postgres no.
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.

php 7.4 Trying to access array offset on value of type null in SlugBehavior
3 participants