Exceptions if slug field not exists or its length is NULL

[burzum]

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]

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]

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]

@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]

@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]

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]

@jadb Sounds good.

[codecov[bot]]

Codecov Report

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

@@             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.